From ebd757d609e98da62fb7e2e0a3e1063e3808a270 Mon Sep 17 00:00:00 2001
From: Martin Stjernholm <mast@google.com>
Date: Fri, 24 May 2019 11:00:30 +0100
Subject: [PATCH] Document how properties work in defaults modules.

Also remove an unused field in DefaultsModuleBase.

Test: m nothing
Test: Verify no diffs in ninja files:
  <check out clean tree>
  m nothing && shasum out/*.ninja | sort -k2 > before
  <apply patch>
  m nothing && shasum out/*.ninja | sort -k2 > after
  diff before after
Bug: 112158820
Change-Id: Id819cea10f7af1603b4e4e1b14c0b49afcd0fecd
---
 android/defaults.go | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/android/defaults.go b/android/defaults.go
index d4fbf487d..e969adc9b 100644
--- a/android/defaults.go
+++ b/android/defaults.go
@@ -63,9 +63,28 @@ func InitDefaultableModule(module DefaultableModule) {
 
 type DefaultsModuleBase struct {
 	DefaultableModuleBase
-	defaultProperties []interface{}
 }
 
+// The common pattern for defaults modules is to register separate instances of
+// the xxxProperties structs in the AddProperties calls, rather than reusing the
+// ones inherited from Module.
+//
+// The effect is that e.g. myDefaultsModuleInstance.base().xxxProperties won't
+// contain the values that have been set for the defaults module. Rather, to
+// retrieve the values it is necessary to iterate over properties(). E.g. to get
+// the commonProperties instance that have the real values:
+//
+//   d := myModule.(Defaults)
+//   for _, props := range d.properties() {
+//     if cp, ok := props.(*commonProperties); ok {
+//       ... access property values in cp ...
+//     }
+//   }
+//
+// The rationale is that the properties on a defaults module apply to the
+// defaultable modules using it, not to the defaults module itself. E.g. setting
+// the "enabled" property false makes inheriting modules disabled by default,
+// rather than disabling the defaults module itself.
 type Defaults interface {
 	Defaultable
 	isDefaults() bool