`misc-const-correctness` suggests `const` on pointer to array even if disabled

[firewave]

static void f()
{
    const char * a[] = {"", ""};
}

<source>:3:5: warning: variable 'a' of type 'const char *[2]' can be declared 'const' [misc-const-correctness]
    const char * a[] = {"", ""};
    ^
                 const

https://godbolt.org/z/e646rPo99

According to https://clang.llvm.org/extra/clang-tidy/checks/misc/const-correctness.html WarnPointersAsValues is disabled by default. It seems this option is not being used when it applies to arrays (that's the only case I have seen it with).

Also the fix-it hint is placed on the existing const which makes this look like a false positive (see the "swiggle" in godbolt). I think that actually should to point before the a instead.

The documentation of WarnPointersAsValues could be bit more straightforward and including a proper before/after example would also be helpful. I knew what it was supposed to do but I really didn't get it from the documentation.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[JonasToth]

Hey, thanks for pointing that out! :)

An array of pointer is not considered a pointer. I have to think about this again if that is proper. I will get back to you.
And yes, documentation as always :)

I am looking into this issue (as well as the other issues you noted!)!

[JonasToth]

1. The check treats arrays as values in all cases. This is a bug or at least unexpected. I would like to have the WarnPointersAsValues be consistent there and not emit warnings for your example => PR coming
2. The transformation for your example is as follows:

   static void f() { const char * const a[] = {"", ""}; }
   

   The transformation is correct and does not break the code.
3. The ^ pointing to the beginning is the warning location, not the fixit-hint insertion place. That is just how clang emits those warnings/fixits. I don't think the warning location should change, as it points to the type in question.

[firewave]

1. The check treats arrays as values in all cases. This is a bug or at least unexpected. I would like to have the WarnPointersAsValues be consistent there and not emit warnings for your example => PR coming

Great.

3. The ^ pointing to the beginning is the warning location, not the fixit-hint insertion place. That is just how clang emits those warnings/fixits. I don't think the warning location should change, as it points to the type in question.

If it starts with static or in this case when the const needs to be placed within the line it can be extremely confusing what is at fault here. Godbolt tagging the first word in the line makes things even "worse" since it might indicate you need to do something with that word to make the warning go away.

But that should be a different issue and is something I wanted to revisit after I went through all the remaining false positives and negatives (one of each so far) I haven't filed issues for yet.

I am also being side tracked as reviewing the findings of this check triggered a lot of other code changes (i.e. objects which were unnecessarily copied instead of being moved).

[JonasToth]

Patch here: https://reviews.llvm.org/D130793

[JonasToth]

If it starts with static or in this case when the const needs to be placed within the line it can be extremely confusing what is at fault here. Godbolt tagging the first word in the line makes things even "worse" since it might indicate you need to do something with that word to make the warning go away.

I understand the confusion and I had to double check that as well. I am not sure how much can be done about it. If the pointer is to variable name, the const would be still somewhere else. But different issue, I agree :)

But that should be a different issue and is something I wanted to revisit after I went through all the remaining false positives and negatives (one of each so far) I haven't filed issues for yet.

Thank you very much! I am hoping to stabilize the check after more real world code has been analyzed. I will prioritize false positives, as they are annoying and wrong transformations make the check much less useful.

Please report false negatives, too ;)

[firewave]

This should probably be backported to 15.x.

[njames93]

I feel there may be a case to remove this check from the 15 branch. And focus on keeping it in the development branch ready for a much cleaner 16 release. @AaronBallman any thoughts about this?

[firewave]

I feel there may be a case to remove this check from the 15 branch.

Why? What "case" is that?

I think this check is more useful and in better shape as some of the ones which have shipped in the past. Yes, this one is rather noisy - but that's by design.

But this check opens up a set of different "issues" if you review the suggested fix-its. Like:

• unnecessary copies (e.g. missing std::move)
• unnecessary variables (e.g. can be inlined/temporary because of single-use)
• too wide a scope
• any "fallback" fixing any of the above has
• ... and possibly something I forgot

I have been struggling cleaning up the code before even applying the suggested fixes permanently (partially because of some weirdness in the code though).
It would be much better if there were existing checks for these as fixing the misc-const-correctness warnings would mask some of those pattern and they could not longer be detected.

So if you make that case this check should not added at all until all those other "issues" which would be masked by the changes could be detected.

In turn it enables other checks like performance-unnecessary-copy-initialization or readability-make-member-function-const to kick in.

So far I have seen two "false positives" (including this one which is technically more of a configuration slip) on the code base I have been running this on. And I still have to look into the other one.

[AaronBallman]

Responding to

This should probably be backported to 15.x.

and

I feel there may be a case to remove this check from the 15 branch. And focus on keeping it in the development branch ready for a much cleaner 16 release. @AaronBallman any thoughts about this?

I think one of these two things should happen. :-)

I remain concerned about the chattiness of this tidy check on real code, but it's less due to these sort of minor bugs and more due to the number of true positives I expect it to give on any existing code base of a decent size (it's a great check for starting a new code base and keeping it clean though). That said, clang-tidy checks can be chatty and this check provides real value to someone who wants it to be enabled. So ultimately, I think the biggest problem is that it's going to be a surprise when upgrading for anyone who enables all checks from misc using a glob. Because the user explicitly said "enable all new checks" and because it's easy enough for them to disable this one manually when using globs, I lean towards leaving the check in 15.x and cherry-picking this fix (presuming it's safe to do so).

If we start to find a bunch of other bugs which suggests there's a serious stability or correctness concern, then I lean more towards pulling it back out of 15.x and trying again for 16.x.