Emit a new warning if such parameters are modified in the body of the
constructor.
Fixes PR16088.
Emit a new warning if such parameters are modified in the body of the
constructor.
Fixes PR16088.
It's not just modifications of shadowed variables that are a problem - one of the one's I'm concerned we should catch is:
struct foo { std::unique_ptr<int> p; foo(std::unique_ptr<int> p) : p(std::move(p)) { f(*p); } };
+Lang, because he was asking me recently about this improvement & thinking
of chipping in
I'm not sure your example is in scope for -Wshadow, though. Any function call that takes a non-const reference to the parameter could modify it. I guess I'm thinking something like:
void trim_in_place(std::string &s); struct A { std::string s; A(std::string s) : s(s) { trim_in_place(s); } };
I think if we try to match that we'll have too many false positives.
I think your example would be better caught by something like -Wconsumed that looks for uses of objects that have been moved-from. That warning won't be confused by shadowing and will give a better diagnostic anyway.
As a data point: I ran -Wshadow on our code base with a similar, but simpler patch (http://reviews.llvm.org/D18395, just disables the warning on ctor parameters named after fields). It removes more than half of the hits (from ~100k initially) and a random sample of ~100 removed removed hits didn't contain any issues that we'd like to still be warning about.
Ultimately, I think, this diagnostic should be made much less noisy to be actually useful. So maybe even starting with a removal of a larger subset of warnings (and then gradually adding back the cases that seem to be important to flag) would be better.
lib/Sema/SemaExpr.cpp | ||
---|---|---|
9858 | Why this change? |
This seems totally out of scope for -Wshadow, and seeing a -Wshadow warning on this wouldn't help (imagine a few tens of lines before the *p is used).
FWIW we don't currently use this warning on Chromium because it's way to noisy. So something like this looks like a great change to me.
dblaikie, are you aware of any codebases that use this warning in its current form?
Not dblaikie, but I've used the GCC version of -Wshadow on reasonably large code bases before. The ctor pattern that this is trying to squelch was certainly a significant portion of the warnings, particularly when (old) boost headers got involved.
I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.
Did squelching this ctor pattern find any bugs? Do you know if boost would miss the warning firing in this case?
I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter, and all uses of the parameter in the constructor body would still be valid if it were declared const".
In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).
(I agree that catching A(X x) : x(std::move(x)) { use(x); } is outside the scope of this change.)
lib/Sema/SemaDecl.cpp | ||
---|---|---|
6418 | Instead of redoing class member lookup every time we check to see if a variable is modifiable, perhaps you could populate a set of shadowed-but-not-diagnosed VarDecl*s here? That way, you can also suppress the duplicate diagnostics if the variable is modified multiple times by removing it from the set. | |
lib/Sema/SemaExpr.cpp | ||
9663 | This query is actually quite expensive (more so than some or all of the other checks below). |
Not dblaikie, but I've used the GCC version of -Wshadow on reasonably large code bases before. The ctor pattern that this is trying to squelch was certainly a significant portion of the warnings, particularly when (old) boost headers got involved.
I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.
Did squelching this ctor pattern find any bugs? Do you know if boost would miss the warning firing in this case?
I don't recall fixing any bugs with the ctor pattern. I can only recall two different classes of things that I have fixed that were flagged with -Wshadow.
I doubt boost would miss the warning, and probably wouldn't even notice (barring overlapping list membership). If you're really wondering if boost would care / notice, @mclow.lists would have a better idea.
That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?
In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).
Would the reference binding check be somewhere in SemaInit.cpp? I don't see an obvious spot to do this check at first glance.
(I agree that catching A(X x) : x(std::move(x)) { use(x); } is outside the scope of this change.)
Wouldn't the proposed check for non-const references find this example, though?
lib/Sema/SemaDecl.cpp | ||
---|---|---|
6418 | That sounds like a good idea. I could even leave it empty if we're not doing -Wshadow. | |
lib/Sema/SemaExpr.cpp | ||
9663 | This check has to be faster than name lookup, right? Maybe do it before that? |
Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck. Consider following examples:
class C1 { T field; public: void set_field(T f) { field = f; // do something else... } C1(T field) { set_field(field); } }; class C2 { T1 f1; T2 f2; public: C2(T1 f1, T2 f2) { // Some form of initialization of `this->field` from `field` that is not just a copy construction/copy/move, // e.g. a method call or an assignment of a single member: this->f1.a = f1.a; this->f2.CopyFrom(f2); } };
I've seen these patterns being used in the real code, and they don't seem to be dangerous or otherwise worth being flagged by this diagnostic.
and all uses of the parameter in the constructor body would still be valid if it were declared const".
That might also be overly restrictive. Consider a variant of set_field above that takes a T& (which is weird, but it doesn't seem like -Wshadow is the right warning in this case).
That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?
In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).
See above. There are many cases, when at least the latter pattern is used in a reasonable way.
Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck.
s/chpatterneck/check/
We had a conversation about this change around the office the other week, and people were concerned about false negatives like the trim_in_place one. Basically, I don't have time to discover all the ways you can modify your parameters:
struct B { A a; B(A a) : a(a) { // modifies parameter, not member a.a = 42; modify_in_place(&a); a.setMember(3); } };
These kinds of bugs are uncommon, but the existing -Wshadow protects users against this today. I think it'll be OK if we give these users an extra -Wshadow-constructor preserves the more strict behavior.
lib/Sema/SemaDecl.cpp | ||
---|---|---|
1615 | This'd be a good place to produce a -Wshadow-constructor or similar warning if D is still in the map. | |
6483 | Maybe map to the canonical decl before using the Decl* as a key? (This doesn't matter for now, but we'll likely want to extend this to other kinds of shadowing in the future.) | |
6489–6490 | Could be useful to also note the location of the shadowing decl. |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
6489–6490 | Sure, done. I reused and renamed a note from uninit variable diagnostics. |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
6428–6436 | I would prefer for us to produce the -Wshadow "modifying shadowed" warning where it applies, even when -Wshadow-all is enabled. The usual model is to act as if -W* flags are just a filter over the diagnostic output, and this diverges from that. Is the goal to ensure that -Wshadow-field-in-constructor diagnoses all constructor parameter / field shadowing, even if -Wshadow is not enabled? |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
6428–6436 | Well, if we do that we'll get two warnings about the shadowed field: one on the site of the parameter declaration and another on the modification. That felt a bit redundant. I can remove the if/else and we'll get the behavior you're asking for. Most users will not use -Wshadow-all, and even if they do they'll only see the double diagnostic in very rare cases. |
lib/Sema/SemaDecl.cpp | ||
---|---|---|
6428–6436 | Here's what I was thinking:
This means that our behavior is as if -W flags filter our warning output, and we only get one warning for each instance of shadowing, and -Wshadow-field-in-constructor can be used to control all constructor-parameter-shadows-field warnings (and even obscure cases like -Wshadow -Wno-shadow-field-in-constructor do what they look like they should do). |