commit cc0e5cf2bc2c480196692b76d8ce5c4268de8e72
author Alex Deymo <deymo@chromium.org> Wed Apr 23 20:20:11 2014 -0700
committer chrome-internal-fetch <chrome-internal-fetch@google.com> Thu Apr 24 23:49:46 2014 +0000
tree 27b5fd2926647b4e417c8573c0f1a5c0e880b5f2
parent 21716e2da121769b4d473c30360e88ef3061de15

PolicyManager: EvaluationContext caches null values.

When a variable fails during an evaluation, we don't want to retry it
on that evaluation. That would imply that the same policy could get
more than one value for that variable (one null and one not null)
which can make bugs harder to find.

Const variables that fail, should fail during the whole program, and
reflect that a given subsystem isn't present on the build.

BUG=None
TEST=unittest modified.

Change-Id: Icb69623318d31883e0a857ce0d1fb900ca68290f
Reviewed-on: https://chromium-review.googlesource.com/196704
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>

policy_manager/evaluation_context-inl.h

diff --git a/policy_manager/evaluation_context-inl.h b/policy_manager/evaluation_context-inl.h
index 4e1922b..c8e3808 100644
--- a/policy_manager/evaluation_context-inl.h
+++ b/policy_manager/evaluation_context-inl.h
@@ -11,9 +11,9 @@
 
 template<typename T>
 const T* EvaluationContext::GetValue(Variable<T>* var) {
-  if (var == NULL) {
+  if (var == nullptr) {
     LOG(ERROR) << "GetValue received an uninitialized variable.";
-    return NULL;
+    return nullptr;
   }
 
   // Search for the value on the cache first.
@@ -24,16 +24,15 @@
   // Get the value from the variable if not found on the cache.
   std::string errmsg;
   const T* result = var->GetValue(RemainingTime(), &errmsg);
-  if (result == NULL) {
+  if (result == nullptr) {
     LOG(WARNING) << "Error reading Variable " << var->GetName() << ": \""
         << errmsg << "\"";
-  } else {
-    // Cache the value for the next time. The map of CachedValues keeps the
-    // ownership of the pointer until the map is destroyed.
-    value_cache_.emplace(
-      static_cast<BaseVariable*>(var),
-      std::move(BoxedValue(result)));
   }
+  // Cache the value for the next time. The map of CachedValues keeps the
+  // ownership of the pointer until the map is destroyed.
+  value_cache_.emplace(
+    static_cast<BaseVariable*>(var),
+    std::move(BoxedValue(result)));
   return result;
 }
 

policy_manager/evaluation_context_unittest.cc

diff --git a/policy_manager/evaluation_context_unittest.cc b/policy_manager/evaluation_context_unittest.cc
index 249a2d5..1ddec84 100644
--- a/policy_manager/evaluation_context_unittest.cc
+++ b/policy_manager/evaluation_context_unittest.cc
@@ -117,16 +117,15 @@
   EXPECT_EQ(42, *p_fake_int);
 }
 
-TEST_F(PmEvaluationContextTest, GetValueDontCacheNULL) {
+TEST_F(PmEvaluationContextTest, GetValueCachesNull) {
   const int* p_fake_int = eval_ctx_->GetValue(&fake_int_var_);
   PMTEST_EXPECT_NULL(p_fake_int);
 
   fake_int_var_.reset(new int(42));
-  // A second attempt to read the variable should work even on the same
-  // EvaluationContext.
+  // A second attempt to read the variable should not work because this
+  // EvaluationContext already got a NULL value.
   p_fake_int = eval_ctx_->GetValue(&fake_int_var_);
-  PMTEST_ASSERT_NOT_NULL(p_fake_int);
-  EXPECT_EQ(42, *p_fake_int);
+  PMTEST_EXPECT_NULL(p_fake_int);
 }
 
 TEST_F(PmEvaluationContextTest, GetValueMixedTypes) {