[PATCH] D79895: Add a new warning to warn when passing uninitialized variables as const reference parameters to a function

[Zequan Wu via Phabricator via cfe-commits]

zequanwu marked an inline comment as done.
zequanwu added inline comments.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1518
   UsesMap uses;
+  UsesMap constRefUses;
 
----------------
rsmith wrote:
> zequanwu wrote:
> > rnk wrote:
> > > If possible, it would be nice to avoid having a second map.
> > I use second map to let the new warning be orthogonal to existing warnings. That means when we have both `-Wuninitialized` and `-Wno-uninitialized-const-reference`, the old test cases about uninitialized variables should all passed. Otherwise, I need to somehow detect the presence of `-Wno-uninitialized-const-reference`, which I don't see a way to do that. Consider this code if we have only one map:
> > ```
> > void consume_const_ref(const int &n);
> > int test_const_ref() {
> >   int n;
> >   consume_const_ref(n);
> >   return n;
> > }
> > ```
> > No matter if the flag`-Wno-uninitialized-const-reference` is present or not, we still need to add `n` in `consume_const_ref(n);` to `uses` map and then let `S.Diag` to decide to diagnose it or not depend on the presence of `-Wno-uninitialized-const-reference`. If the flag is given, `S.Diag` will just ignore the warning, but uninit var warning will diagnose if `Use.getKind()` returns `Always` in https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/AnalysisBasedWarnings.cpp#L814 . That is not desired. Since the original behavior is to ignore the `n` in `consume_const_ref(n);` by not adding it to the `uses` map.
> > 
> > If there is a way to detect the presence of `-Wno-uninitialized-const-reference` or to know if `Sema::Diag(SourceLocation Loc, unsigned DiagID)` actually diagnosed a warning, the use of second map can be avoid. 
> Interesting testcase :)
> 
> Ideally we want the set of diagnostics from a particular compilation to be the same you'd get by building with `-Weverything` and then filtering out the ones you didn't want. So it's better to not ask whether `-Wuninitialized-const-reference` is enabled or not.
> 
> I think we only populate this map after we've found an uninitialized use; if so, the cost of a second map seems likely to be acceptable.
> 
> FYI, there is a mechanism to ask the "is this diagnostic enabled?" question, but generally we'd prefer if it's only used to avoid doing work that would be redundant if the warning is disabled. You can use `Diag.isIgnored(diag::warn_..., Loc)` to determine if a diagnostic with the given ID would be suppressed at the given location. (In fact, it looks like this is already being called in a later change in this file.)
I think second map is necessary. So, `Diag.isIgnored` is not abused.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1590-1600
+    // flush all const reference uses diags
+    for (const auto &P : constRefUses) {
+      const VarDecl *vd = P.first;
+      const MappedType &V = P.second;
+
+      UsesVec *vec = V.getPointer();
+      for (const auto &U : *vec) {
----------------
rsmith wrote:
> Do we want any idiomatic-self-init special-case handling here? For example:
> 
> ```
> void f(const int&);
> void g() {
>   int a = a;
>   f(a);
> }
> ```
> 
> Following the logic above, should that warn on the `int a = a;` not on the `f(a)` call? Or should we warn only on the `f(a)` call itself in this case? It seems like it might be surprising to say that `a` is "uninitialized" here, since an initializer was provided, even though it was a garbage one.
For this case, I think we should warn at `int a = a`, like the comment in `DiagnoseUninitializedUse` said, https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/AnalysisBasedWarnings.cpp#L986-L996

`f(a)` is considered as accessing `a`.


================
Comment at: clang/test/SemaCXX/warn-uninitialized-const-reference.cpp:23
+    int k = const_use(k); // expected-warning {{variable 'k' is uninitialized when used within its own initialization}}
+    A a2 = const_use_A(a2); // expected-warning {{variable 'a2' is uninitialized when used within its own initialization}}
+    A a3(const_ref_use_A(a3)); // expected-warning {{variable 'a3' is uninitialized when passes as a const reference parameter here}}
----------------
aeubanks wrote:
> For my knowledge, is this to make sure that this other warning takes precedence over the one introduced in this change? If it is, a comment would be nice.
No, original behavior of `-Wuninitialized` for line 24 is silence. Now the new warning will be emitted because it is const use of uninitialized variable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79895/new/

https://reviews.llvm.org/D79895