In D73464#1844402, @f00kat wrote:

In D73464#1844310, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf?

Yes, please. I don`t know how :)

In D73457#1844475, @thakis wrote:

Since we auto-detect -fmsc-version if it's not explicitly installed and since this warning is on by default, it makes the test suite depend on the environment a good bit. Given how old 2015 is by now, I'm not sure this complexity and subtlety is worth the benefit?

Committed in 554791928088d6139e0fb3480d79cd76ea59198f

In D72217#1844262, @sammccall wrote:

In D72217#1844204, @njames93 wrote:

https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

// Copy pointers, but make it clear that they're pointers.
for (const auto *Ptr : Container) { observe(*Ptr); }
for (auto *Ptr : Container) { Ptr->change(); }

This is the reasoning behind putting the const qualifier in the check. If people feel that this isn't quite enough to justify the const qualifier I'll submit a follow up. The general consensus though is that you should mark auto pointers as const if they don't need to change to express intent

The text/rule there is explicitly about avoiding/clarifying copies - the examples indeed use 'const' but AFAICT the "don't copy" reasoning only applies to including *&.

FWIW I think const here is often noise, particularly in AST-walking code where you're traversing an edge from an X* to a Y* - the latter will be const if the former is, and I care at API boundaries but not in between. (It's usually a meaningless distinction - e.g. we're just reading but it's a non-const pointer because RecursiveASTVisitor isn't const-friendly).

So while spelling const is often helpful, we shouldn't (and don't) require it, and the current config of this check is too intrusive.

In D72448#1844320, @njames93 wrote:

In D72448#1844309, @aaron.ballman wrote:

In D72448#1844032, @njames93 wrote:

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

Designed to be macro unsafe, or just didn't consider macros in the first place? I'm not seeing anything that makes me think macros were taken into account, but maybe you're looking at something different from me. Do you have an example of where you think something was designed to be macro unsafe?

This is one of the test cases

#define M(x) x
#define N { std::string s = ""; }
// CHECK-FIXES: #define N { std::string s = ""; }

void h() {
  templ<int>();
  templ<double>();

  M({ std::string s = ""; })
  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
  // CHECK-FIXES: M({ std::string s; })

  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
}
// further down
#define EMPTY_STR ""
void j() {
  std::string a(EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string a;
  std::string b = (EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string b;

Looks like they knew the behaviour they wanted back then

In D72448#1844032, @njames93 wrote:

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

LGTM! Do you need someone to commit on your behalf?

LGTM!

LGTM!

In D73457#1842605, @amccarth wrote:

In D73457#1842493, @simon_tatham wrote:

Removed the special case for MSCompatibilityVersion == 0. If the default compatibility setting needs to be changed, that's a separate piece of work and should be done by someone who understands more than I do about all those failing tests :-) The new version of this patch just uses the existing default.

+1. The issue of how the default version gets set is a separate issue. I think it's best to keep this patch decoupled from that.

Adding some more Windows reviewers to see if there are more opinions on how to handle this.

LGTM aside from some minor nits.

You should also regenerate the AST matcher documentation by running clang\docs\tools\dump_ast_matchers.py

Thank you for the work on this!

In D73007#1839648, @aaronpuchert wrote:

Here is a proposal: we add two children to -Wrange-loop-analysis.

• -Wrange-loop-construct warns about possibly unintended constructor calls. This might be in -Wall. It contains
  • warn_for_range_copy: loop variable A of type B creates a copy from type C
  • warn_for_range_const_reference_copy: loop variable A is initialized with a value of a different type resulting in a copy
• -Wrange-loop-bind-ref[erence] warns about misleading use of reference types. This might not be in -Wall. It contains
  • warn_for_range_variable_always_copy: loop variable A is always a copy because the range of type B does not return a reference

LGTM aside from some minor nits.

LGTM, but I'll echo what @rjmccall said about this level of granularity being a bit too fine for review for future patches (we expect changes to be testable, which this is, but only if you also review other patches and notice the tests and how they relate to this patch, which can be easy for reviewers to miss).

In D72378#1840461, @lebedev.ri wrote:

In D72378#1840455, @aaron.ballman wrote:

In D72378#1840353, @lebedev.ri wrote:

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

Doesn't the AllowedIdentifiers option work for you?

Uuuh. To be honest somehow i did not observe it when looking through
the docs (as indicated in it since there is no whitelist..),
It does work in this case, thanks. Sorry for false alarm.

In D72378#1840353, @lebedev.ri wrote:

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

In D73028#1839572, @rsmith wrote:

In D73028#1839494, @steveire wrote:

In D73028#1839383, @rsmith wrote:

The follow-up is here: https://reviews.llvm.org/D73029 .

I have the need to change the ascending traversal implemented in ASTContext.cpp with a map (populated while descending) to make it skip nodes too during parent traversal.

I didn't want to have descending traversal in Expr.cpp and ascending traversal in ASTContext.cpp as they would be likely to go out of sync, so moving this implementation here allows extending the purpose in the follow-up.

The ascending traversal should not be in the AST library at all. If tooling or static analysis wants this, that's fine, but navigating upwards is not something the AST supports, and we do not want any part of clang proper developing a reliance on having a parent map, so we're working on moving the parent map out of ASTContext (with a plan to eventually move it out of the AST library). See D71313 for related work in this area.

Continues to LG, do you need me to commit on your behalf?

There should also be a mention of this in the release notes (especially if the default behavior winds up changing).

Test cases?

This looks like it's an NFC patch where the only change is to hoist this functionality out into its own interface. Is that correct? If so, can you please be sure to add NFC to the commit log?

In D73007#1837621, @rtrieu wrote:

I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall.

In D71566#1834472, @serge-sans-paille wrote:

In D71566#1832394, @aaron.ballman wrote:

(There are still some minor whitespace nits to resolve as well.)

Strange, everything is passed through clang-format-diff :-/

In D72373#1834591, @AlexanderLanin wrote:

Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in misc-misplaced-const.cpp - I really do not understand how that happened.
Rebased and verified with check-clang-tools.

I've commit in 968561bcdc34c7d74482fe3bb69a045abf08d2c1 (clang) and 968561bcdc34c7d74482fe3bb69a045abf08d2c1 (llvm) and will keep an eye on the bots to revert if needed. Thanks!

In D73098#1832489, @njames93 wrote:

In D73098#1832131, @Mordante wrote:

Would it make sense to also allow wmain with wchar_t? https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019

Does wmain get used in a lot of projects. If it's very niche then I don't feel it warrants a place in here. If it does I'll add it in.

In D72373#1833569, @thakis wrote:

Looks like this breaks tests: http://45.33.8.238/linux/8102/step_8.txt

I've commit on your behalf in ecc7dae50c41bc8a129a158ecf0ae0270126505c. Sorry about the delay in committing and thank you for the patch!

(There are still some minor whitespace nits to resolve as well.)

In D72635#1831782, @eti-p-doray wrote:

Thanks!
I don't I have commit access. Can you commit on my behalf?

LGTM!

In D71607#1828055, @sorenj wrote:

Okay, but as you can see the majority of my test cases are intentionally false negatives - -Wsign-conversion triggers so often than many people don't use it.

LGTM! Do you think this should also be pushed onto the release branch?

Btw, do you need me to commit this on your behalf, or have you obtained your commit privileges?

LGTM with a minor testing request, thank you!

In rGbcda877b43093459dc52747d35abe0019f8e22e0#885466, @logan-5 wrote:

Correct, it's not supported for MSVC yet. I just tried to enable it to see if MSVC 2019 has support and I get errors:

C:\llvm-project\llvm\include\llvm\ADT\Optional.h(290): error C2560: 'llvm::Optional<unknown-type> llvm::Optional<T>::map(const Function &) &&': cannot overload a member function with ref-qualifier with a member function without ref-qualifier

Perhaps the map overload just above that simply needs LLVM_LVALUE_FUNCTION on it? Like how the other getValueOr() and operator*s do.

LGTM

In rGbcda877b43093459dc52747d35abe0019f8e22e0#885457, @logan-5 wrote:

That's a bummer. I wrote *std::move(Fixup) intentionally that way, since llvm::Optional has a &&-qualified operator* overload that returns std::move of the contained value. I guess the Windows configuration doesn't have LLVM_HAS_RVALUE_REFERENCE_THIS?

Thank you for working on this, I think this is a great improvement. I am not super qualified to review the cmake, but it looks eminently reasonable to me.

In D72635#1825219, @aaronpuchert wrote:

Hmm, I have been wondering about this as well. The way I see it, all of these things are what we call capabilities, and we treat them all the same. The names are just meant to make warning messages more readable, because what the analysis considers a capability, the user might know as a mutex, or role, or sequence.

Thank you for the new checker -- I've committed on your behalf in 42a0355816d3bc125d59cbd07052c8886e78ca86

Thank you for the patch, I've commit on your behalf in d5c6b8407c12d39a78f42a216369407cb2d7b511

In D72378#1824586, @logan-5 wrote:

Great! In that case, I'll need help committing this, as well as the thing it depends on, https://reviews.llvm.org/D72284 (which has also been LGTM'd).

In D72635#1822686, @eti-p-doray wrote:

I added an example in the description.

From doc https://clang.llvm.org/docs/ThreadSafetyAnalysis.html,
it sounds like we should be allowed to declare our class with CAPABILITY("context"), but it turns out that only "mutex" and "role" are allowed.
I could otherwise update this CL to allow any string (single word lowercase?) as CAPABILITY?

In D71707#1821903, @jankratochvil wrote:

In D71707#1796671, @aaron.ballman wrote:

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?

I think only __uptr support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?

LGTM with a few nits.

LGTM!

Given that the compiler already has -Wsign-conversion, I'm not certain what value is added by this check. Can you explain a bit about why this is the correct approach for diagnosing the issue? When you ignore the false positives from the test, the only cases not pointed out by -Wsign-compare are the macro cases (which is reasonable behavior to not diagnose on -- the fix for one macro may make other macros invalid).

In D72373#1809080, @AlexanderLanin wrote:

FIX-IT isn't quite that obvious. Some options:

• look for other typedef which contains the same as const
• create new typedef/using
• remove "*" from typedef and adjust all usage accordingly. Implies removing "Ptr" suffix.

Can you give some examples of code where you think the diagnostics are inappropriate? One of the goals of role-based capabilities is for you to name the capabilities as you see fit, so it's possible that there is a different way for us to surface those diagnostics for your situation that doesn't involve adding another one-off name for capabilities.

Aside from changing the attribute name back to what it was (sorry about that), this LGTM!

Thank you for the patch, I've committed on your behalf in ee0f1f1edc3ec0d4e698d50cc3180217448802b7

In D68115#1820622, @vitalybuka wrote:

In D68115#1820579, @aaron.ballman wrote:

In D68115#1820462, @vitalybuka wrote:

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

In general, I believe so, yes. To be clear, it only sets zeros into union padding, not *all* padding. I do not have an opinion on whether we want it to be -fzero-union-padding as opposed to -finit-union-padding that honors the pattern from -ftrivial-auto-init=pattern and defaults to zero if no pattern is specified.

They whole point of the patch was to avoid breaking code by -ftrivial-auto-init=pattern with "MyUnion my_union = {}". So to fix that only -fzero-union-padding behavior helpful.
-ftrivial-auto-init=pattern as-is already inits union padding with patterns.

In D68115#1820462, @vitalybuka wrote:

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

Thank you for the new check, I've commit on your behalf in 36fcbb838c8f293f46bfed78c6ed8c177f1e3485

In D72217#1819678, @njames93 wrote:

In D72217#1819437, @aaron.ballman wrote:

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

For now I would appreciate it, I have fired off an email though, cheers.

LGTM!

The changes LGTM, but I notice that our DR status page says we already supported CWG2292 as of Clang 9 (Richard changed the status in svn:368941) which surprises me. Was this aspect just missed with the original commit, or is something else going on?

I think this patch is getting pretty close to finished -- have you tried running it over a large corpus of code to see if it spots reserved identifiers (or unreserved ones)? If not, I would recommend running it over LLVM + clang + clang-tools-extra to see how it operates when looking for reserved identifiers and libc++ for unreserved ones.

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

In D68115#1811091, @lebedev.ri wrote:

In D68115#1811089, @hubert.reinterpretcast wrote:

In D68115#1810891, @lebedev.ri wrote:

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887);

Would be great if @rsmith / @aaron.ballman could comment on that