Places where we have const* arguments in source, optionally mark the value as const as well. by ahsonkhan · Pull Request #1261 · Azure/azure-sdk-for-c

ahsonkhan · 2020-09-12T01:44:15Z

Used this regex:
const\* (?!const) -> const* const on *.c files excluding sdk\tests, sdk\samples, and intentionally skipping *.h files

value as const as well.

momuno

Side Q: I'm intrigued that this compiles without changing header declarations. Do you know why that is?

sdk/src/azure/core/az_log.c

sdk/src/azure/core/az_context.c

sdk/src/azure/core/az_http_policy_logging.c

ahsonkhan · 2020-09-14T19:38:05Z

Side Q: I'm intrigued that this compiles without changing header declarations. Do you know why that is?

I will quote @BillyONeal

in C, f(char*) and f(char* const) are the same function

I don't know the underlying compiler or language reason to exactly why that is (or the mechanism behind it), but if I had to guess, the generated assembly is going to be identical and the second const is just an implementation detail hint used by the compiler in some way to validate assignments.

BillyONeal · 2020-09-14T20:21:32Z

I don't know the underlying compiler or language reason to exactly why that is

C11 6.7.6.3 "Function declarators (including prototypes)"/15:

The part I highlighted says that type qualifiers are removed from the declared type when determining whether function types are compatible. Note that the const in const T* is not a qualifier on the pointer's type, it's on the pointee's type. C only strips the top level qualifier, it doesn't "reach inside" to remove the inner const.

momuno · 2020-09-14T23:13:12Z

I don't know the underlying compiler or language reason to exactly why that is

C11 6.7.6.3 "Function declarators (including prototypes)"/15:

The part I highlighted says that type qualifiers are removed from the declared type when determining whether function types are compatible. Note that the const in const T* is not a qualifier on the pointer's type, it's on the pointee's type. C only strips the top level qualifier, it doesn't "reach inside" to remove the inner const.

Thanks for this info. A spot I'm still a little confused on is the removing of qualifiers to determine if function types are compatible. Here's my question: If the qualifier (in this case const on the pointee) is removed prior to compatibility testing, then wouldn't that make int function_name(int const* var1) the same as int function_name(int* var1) ? However, I have found that the compiler will complain if the decalaration and definition of a function differ in this respect.

momuno · 2020-09-14T23:15:05Z

@ahsonkhan are you thinking to also update the header files in a separate PR?

BillyONeal · 2020-09-14T23:23:26Z

If the qualifier (in this case const on the pointee)

Qualifiers on the pointee aren't qualifiers on the function argument, so they are not adjusted by the above wording.

then wouldn't that make int function_name(int const* var1) the same as int function_name(int* var1) ?

No, because the const in "int const*" is not a type qualification of the function parameter. The function parameter's type is a pointer, not a const or non-const int. The const in int *const is a type qualification of the function parameter, since the const refers to the function's parameter, which is a pointer.

momuno · 2020-09-14T23:57:15Z

If the qualifier (in this case const on the pointee)

Qualifiers on the pointee aren't qualifiers on the function argument, so they are not adjusted by the above wording.

then wouldn't that make int function_name(int const* var1) the same as int function_name(int* var1) ?

No, because the const in "int const*" is not a type qualification of the function parameter. The function parameter's type is a pointer, not a const or non-const int. The const in int *const is a type qualification of the function parameter, since the const refers to the function's parameter, which is a pointer.

Oh I get it now. I had it flipped. Thank you!

ahsonkhan · 2020-09-15T00:44:51Z

are you thinking to also update the header files in a separate PR?

No. That is intentionally not something we want to do as per our discussion/meeting last Friday. We don't want redundant use of const in the public headers to increase cognitive burden on the end user. Having it optionally used in the source files, to try to validate correctness, is OK.

Quoting @CIPop

In SRC we can have the const on all parameters that are const in this version of the implementation. The public API (include files) do not need to have the consts since they do not affect in any way the application (taking a dependency on our sources).

http://www.gotw.ca/gotw/006.htm
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#exception-49
https://stackoverflow.com/questions/117293/use-of-const-for-function-parameters/11036371#11036371
https://stackoverflow.com/questions/1554750/c-const-keyword-use-liberally/1554820#1554820

momuno · 2020-09-15T09:24:22Z

are you thinking to also update the header files in a separate PR?

No. That is intentionally not something we want to do as per our discussion/meeting last Friday. We don't want redundant use of const in the public headers to increase cognitive burden on the end user. Having it optionally used in the source files, to try to validate correctness, is OK.

Quoting @CIPop

In SRC we can have the const on all parameters that are const in this version of the implementation. The public API (include files) do not need to have the consts since they do not affect in any way the application (taking a dependency on our sources).

http://www.gotw.ca/gotw/006.htm
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#exception-49
https://stackoverflow.com/questions/117293/use-of-const-for-function-parameters/11036371#11036371
https://stackoverflow.com/questions/1554750/c-const-keyword-use-liberally/1554820#1554820

Sounds good. Tbh, there was so much back and forth, repetition and circling back around in that debate, I wasn't clear on the end result.

ahsonkhan · 2020-09-17T07:09:34Z

@RickWinter, @ericwol-msft, @barcharcraz, @CIPop - please review.

…o UseConstInSrc

ahsonkhan · 2020-09-17T23:45:46Z

Got confirmation from @ericwol-msft offline that this should wait until after GA. Please hold off on merging for now.

…o UseConstInSrc

ghost · 2020-09-24T00:27:06Z

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…ark the value as const as well. (Azure#1261)" This reverts commit 5723678.

…ark the value as const as well. (#1261)" (#1517) This reverts commit 5723678.

* Make sure we always log in as app or provisioner (#1480) This also adds previously-required parameters back into @PSBoundParameters to pass down to pre- and post-scripts. Co-authored-by: Heath Stewart <heaths@microsoft.com> * update changelog for beta 2 (#1484) * Sync eng/common directory with azure-sdk-tools for PR 1163 (#1483) * Update subscription configuration schema to include new parameters * Support platform specific arm template parameters and legacy hashtable format * Update arm template parameter comment to include top level key * Restore AdditionalParameters. Merge ArmTemplateParameters from stringified hash literal * Handle duplicate keys more explicitly for arm and env vars * Regenerate New-TestResources.ps1 markdown * revert variable name to environmentVariables to fix post-scripts * Handle empty arm template parameters better * Remove arm template parameter merge logic from deploy template * Add merge hashes function to New-TestResources.ps1 * Add merge hashes function to New-TestResources.ps1 * Add env variable overwrite warning. Use ContainsKey checks * Temporarily manually fix invalid generated markdown links Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * Sync eng/common directory with azure-sdk-tools for PR 1178 (#1487) * Refactor eng/common/README.md * Add doc directory Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> * Fix broken eng/common doc link (#1489) Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> * Explicitly ignore return of _az_span_token on c2d parsing (#1482) (#1490) * add latest static analyzer fix to changelog (#1491) * Sync eng/common directory with azure-sdk-tools for PR 1188 (#1502) * Add debug flag to arm deployment command * Only set debug preference when $CI is true Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * Remove invalid characters in basename sourced from username (#1503) Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * Sync eng/common directory with azure-sdk-tools for PR 1170 (#1481) * Added the preprocess scripts. * string array to string Co-authored-by: Sima Zhu <sizhu@microsoft.com> * Reorganization of samples readme (#1505) * Completion of Sample Readme update (#1508) * Add Invoke-DevOpsAPI.ps1, Add functions for Canceling and Listing Builds (#1474) Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> * Change live test resource DeleteAfterHours tag to 8 hours (#1509) Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * adding notes about security on readme file (#1498) * adding notes about security on readme file * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Mollie Munoz <mollie.munoz@microsoft.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Ahson Khan <ahson_ahmedk@yahoo.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Ahson Khan <ahson_ahmedk@yahoo.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Mollie Munoz <mollie.munoz@microsoft.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Ahson Khan <ahson_ahmedk@yahoo.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Mollie Munoz <mollie.munoz@microsoft.com> * Update how_to_iot_hub_esp8266_nodemcu.md * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Eric Wolz <ericwol@microsoft.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Eric Wolz <ericwol@microsoft.com> * Update sdk/samples/iot/docs/how_to_iot_hub_esp8266_nodemcu.md Co-authored-by: Eric Wolz <ericwol@microsoft.com> Co-authored-by: Mollie Munoz <mollie.munoz@microsoft.com> Co-authored-by: Ahson Khan <ahson_ahmedk@yahoo.com> Co-authored-by: Eric Wolz <ericwol@microsoft.com> * Sync eng/common directory with azure-sdk-tools for PR 1202 (#1510) * Add debugging link on resource deployment failures to log output * Update aka link for live test help docs. Use here string and empty throw. Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * Sync eng/common directory with azure-sdk-tools for PR 1153 (#1513) * Improve Update-ChangeLog Logic * Updates to ChangeLog-Operations.ps1, copy-docs-to-blobstorage.ps1, Invoke-GitHubAPI.ps1 and Package-Properties.ps1 * More changeLog Logic Improvements * Update date parsing Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> * Revert "Places where we have const* arguments in source, optionally mark the value as const as well. (#1261)" (#1517) This reverts commit 5723678. * Update AddAzureSDKforC.cmake to reference the latest tagged release of the SDK (#1495) * Update AddAzureSDKforC.cmake to reference the latest release of the SDK * Remove pnp from the tag being referenced. * Incrementing master CL and version to be above the next release from the feature branch (#1506) * Update LanguageSetting.ps1 (#1518) Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: Heath Stewart <heaths@microsoft.com> Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> Co-authored-by: ewertons <ewertons@microsoft.com> Co-authored-by: Sima Zhu <sizhu@microsoft.com> Co-authored-by: Mollie Munoz <mollie.munoz@microsoft.com> Co-authored-by: Wellington Duraes <wellington.duraes@gmail.com> Co-authored-by: Ahson Khan <ahson_ahmedk@yahoo.com> Co-authored-by: Eric Wolz <ericwol@microsoft.com> Co-authored-by: Chidozie Ononiwu <31145988+chidozieononiwu@users.noreply.github.com>

Places where we have const* arguments in source, optionally mark the

38c9764

value as const as well.

ahsonkhan added this to the [2020] October milestone Sep 12, 2020

ahsonkhan requested review from CIPop, JeffreyRichter, RickWinter, antkmsft, danewalton-msft, ericwolz, ewertons, hihigupt, jspaith, momuno and vhvb1989 as code owners September 12, 2020 01:44

ahsonkhan self-assigned this Sep 12, 2020

momuno reviewed Sep 14, 2020

View reviewed changes

sdk/src/azure/core/az_log.c Outdated Show resolved Hide resolved

sdk/src/azure/core/az_context.c Show resolved Hide resolved

sdk/src/azure/core/az_context.c Show resolved Hide resolved

sdk/src/azure/core/az_http_policy_logging.c Show resolved Hide resolved

vhvb1989 approved these changes Sep 14, 2020

View reviewed changes

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-c int…

41c1f5b

…o UseConstInSrc

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-c int…

5fe0517

…o UseConstInSrc

ahsonkhan added the auto-merge label Sep 24, 2020

Auto-format the files.

dc6924a

ghost merged commit 5723678 into Azure:master Sep 24, 2020

ahsonkhan deleted the UseConstInSrc branch September 24, 2020 01:17

This was referenced Nov 13, 2020

Compiler warnings: formal parameter different from declaration #1512

Closed

Address right hand const on API source files #1516

Closed

danewalton-msft added a commit to danewalton-msft/azure-sdk-for-c that referenced this pull request Nov 13, 2020

Revert "Places where we have const* arguments in source, optionally m…

9db1d8f

…ark the value as const as well. (Azure#1261)" This reverts commit 5723678.

danewalton-msft mentioned this pull request Nov 13, 2020

Revert "Places where we have const* arguments in source, optionally m… #1517

Merged

danewalton-msft added a commit that referenced this pull request Nov 14, 2020

Revert "Places where we have const* arguments in source, optionally m…

4b2f787

…ark the value as const as well. (#1261)" (#1517) This reverts commit 5723678.

This pull request was closed.

Conversation

ahsonkhan commented Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

momuno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BillyONeal commented Sep 14, 2020

Uh oh!

momuno commented Sep 14, 2020

Uh oh!

momuno commented Sep 14, 2020

Uh oh!

BillyONeal commented Sep 14, 2020

Uh oh!

momuno commented Sep 14, 2020

Uh oh!

ahsonkhan commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

momuno commented Sep 15, 2020

Uh oh!

ahsonkhan commented Sep 17, 2020

Uh oh!

ahsonkhan commented Sep 17, 2020

Uh oh!

ghost commented Sep 24, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahsonkhan commented Sep 12, 2020 •

edited

Loading

ahsonkhan commented Sep 14, 2020 •

edited

Loading

ahsonkhan commented Sep 15, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.