diff mbox

[2/2] kconfig: fix comment for symbol flag SYMBOL_AUTO

Message ID 20180629091237.16620-3-dirk@gouders.net (mailing list archive)
State New, archived
Headers show

Commit Message

Dirk Gouders June 29, 2018, 9:12 a.m. UTC
I could not verify the comment for that symbol flag.

I could only find that flag set for choices and the defconfig_list
symbol in a dump of all symbols, which corresponds to the only two
locations in the code where that flag is being set explicitely.

Signed-off-by: Dirk Gouders <dirk@gouders.net>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 scripts/kconfig/expr.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Masahiro Yamada July 2, 2018, 3:14 p.m. UTC | #1
2018-06-29 18:12 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
> I could not verify the comment for that symbol flag.

Good catch.
I forgot to fix up the comment
in commit 104daea149c4.


> I could only find that flag set for choices and the defconfig_list
> symbol in a dump of all symbols, which corresponds to the only two
> locations in the code where that flag is being set explicitely.
>
> Signed-off-by: Dirk Gouders <dirk@gouders.net>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  scripts/kconfig/expr.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index 94a383b21df6..0f53e44f14d6 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -141,7 +141,9 @@ struct symbol {
>  #define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
>  #define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
>  #define SYMBOL_CHANGED    0x0400  /* ? */
> -#define SYMBOL_AUTO       0x1000  /* value from environment variable */
> +#define SYMBOL_AUTO       0x1000  /* Symbols of type choice and the
> +                                  * symbol with option defconfig_list
> +                                  * have this flag set */

Hmm.  This explanation is not very helpful in my opinion.
Could you reword that?

In my understanding, symbols with SYMBOL_AUTO
are never written out to file.





>  #define SYMBOL_CHECKED    0x2000  /* used during dependency checking */
>  #define SYMBOL_WARNED     0x8000  /* warning has been issued */
>
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dirk Gouders July 3, 2018, 7:32 a.m. UTC | #2
Masahiro Yamada <yamada.masahiro@socionext.com> writes:

> 2018-06-29 18:12 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
>> I could not verify the comment for that symbol flag.
>
> Good catch.
> I forgot to fix up the comment
> in commit 104daea149c4.
>
>
>> I could only find that flag set for choices and the defconfig_list
>> symbol in a dump of all symbols, which corresponds to the only two
>> locations in the code where that flag is being set explicitely.
>>
>> Signed-off-by: Dirk Gouders <dirk@gouders.net>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> ---
>>  scripts/kconfig/expr.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
>> index 94a383b21df6..0f53e44f14d6 100644
>> --- a/scripts/kconfig/expr.h
>> +++ b/scripts/kconfig/expr.h
>> @@ -141,7 +141,9 @@ struct symbol {
>>  #define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
>>  #define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
>>  #define SYMBOL_CHANGED    0x0400  /* ? */
>> -#define SYMBOL_AUTO       0x1000  /* value from environment variable */
>> +#define SYMBOL_AUTO       0x1000  /* Symbols of type choice and the
>> +                                  * symbol with option defconfig_list
>> +                                  * have this flag set */
>
> Hmm.  This explanation is not very helpful in my opinion.
> Could you reword that?
>
> In my understanding, symbols with SYMBOL_AUTO
> are never written out to file.

Yes, that's right, sym_calc_value() clears SYMBOL_WRITE for those
symbols and the comment should describe the effect of the flag, not it's
users.  I will read a bit more to see if the flag has more effects and
reword the comment.

Sidenote: probably, AUTO is a misnomer for this flag.  I would expect
something like automatically generated symbols but the symbols that have
this flag set are rather -- hmm, perhaps auxiliary symbols?

Dirk
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Masahiro Yamada July 3, 2018, 7:51 a.m. UTC | #3
2018-07-03 16:32 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>
>> 2018-06-29 18:12 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
>>> I could not verify the comment for that symbol flag.
>>
>> Good catch.
>> I forgot to fix up the comment
>> in commit 104daea149c4.
>>
>>
>>> I could only find that flag set for choices and the defconfig_list
>>> symbol in a dump of all symbols, which corresponds to the only two
>>> locations in the code where that flag is being set explicitely.
>>>
>>> Signed-off-by: Dirk Gouders <dirk@gouders.net>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> ---
>>>  scripts/kconfig/expr.h | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
>>> index 94a383b21df6..0f53e44f14d6 100644
>>> --- a/scripts/kconfig/expr.h
>>> +++ b/scripts/kconfig/expr.h
>>> @@ -141,7 +141,9 @@ struct symbol {
>>>  #define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
>>>  #define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
>>>  #define SYMBOL_CHANGED    0x0400  /* ? */
>>> -#define SYMBOL_AUTO       0x1000  /* value from environment variable */
>>> +#define SYMBOL_AUTO       0x1000  /* Symbols of type choice and the
>>> +                                  * symbol with option defconfig_list
>>> +                                  * have this flag set */
>>
>> Hmm.  This explanation is not very helpful in my opinion.
>> Could you reword that?
>>
>> In my understanding, symbols with SYMBOL_AUTO
>> are never written out to file.
>
> Yes, that's right, sym_calc_value() clears SYMBOL_WRITE for those
> symbols and the comment should describe the effect of the flag, not it's
> users.  I will read a bit more to see if the flag has more effects and
> reword the comment.
>
> Sidenote: probably, AUTO is a misnomer for this flag.

I agree.


> I would expect
> something like automatically generated symbols but the symbols that have
> this flag set are rather -- hmm, perhaps auxiliary symbols?

Good.
Another idea might be SYMBOL_NO_WRITE ??
(it seems almost self-commenting)
Dirk Gouders July 3, 2018, 9:37 a.m. UTC | #4
Masahiro Yamada <yamada.masahiro@socionext.com> writes:

> 2018-07-03 16:32 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
>> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>>
>>> 2018-06-29 18:12 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
>>>> I could not verify the comment for that symbol flag.
>>>
>>> Good catch.
>>> I forgot to fix up the comment
>>> in commit 104daea149c4.
>>>
>>>
>>>> I could only find that flag set for choices and the defconfig_list
>>>> symbol in a dump of all symbols, which corresponds to the only two
>>>> locations in the code where that flag is being set explicitely.
>>>>
>>>> Signed-off-by: Dirk Gouders <dirk@gouders.net>
>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>>> ---
>>>>  scripts/kconfig/expr.h | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
>>>> index 94a383b21df6..0f53e44f14d6 100644
>>>> --- a/scripts/kconfig/expr.h
>>>> +++ b/scripts/kconfig/expr.h
>>>> @@ -141,7 +141,9 @@ struct symbol {
>>>>  #define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
>>>>  #define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
>>>>  #define SYMBOL_CHANGED    0x0400  /* ? */
>>>> -#define SYMBOL_AUTO       0x1000  /* value from environment variable */
>>>> +#define SYMBOL_AUTO       0x1000  /* Symbols of type choice and the
>>>> +                                  * symbol with option defconfig_list
>>>> +                                  * have this flag set */
>>>
>>> Hmm.  This explanation is not very helpful in my opinion.
>>> Could you reword that?
>>>
>>> In my understanding, symbols with SYMBOL_AUTO
>>> are never written out to file.
>>
>> Yes, that's right, sym_calc_value() clears SYMBOL_WRITE for those
>> symbols and the comment should describe the effect of the flag, not it's
>> users.  I will read a bit more to see if the flag has more effects and
>> reword the comment.
>>
>> Sidenote: probably, AUTO is a misnomer for this flag.
>
> I agree.
>
>
>> I would expect
>> something like automatically generated symbols but the symbols that have
>> this flag set are rather -- hmm, perhaps auxiliary symbols?
>
> Good.
> Another idea might be SYMBOL_NO_WRITE ??
> (it seems almost self-commenting)

Oh sorry, there was an overlap with me sending out v2 without
recognizing and reading your reply.

I like SYMBOL_NO_WRITE, and would prepare v3 if nobody has objections.

Dirk
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index 94a383b21df6..0f53e44f14d6 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -141,7 +141,9 @@  struct symbol {
 #define SYMBOL_OPTIONAL   0x0100  /* choice is optional - values can be 'n' */
 #define SYMBOL_WRITE      0x0200  /* write symbol to file (KCONFIG_CONFIG) */
 #define SYMBOL_CHANGED    0x0400  /* ? */
-#define SYMBOL_AUTO       0x1000  /* value from environment variable */
+#define SYMBOL_AUTO       0x1000  /* Symbols of type choice and the
+				   * symbol with option defconfig_list
+				   * have this flag set */
 #define SYMBOL_CHECKED    0x2000  /* used during dependency checking */
 #define SYMBOL_WARNED     0x8000  /* warning has been issued */