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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks accepted this revision.
aeubanks added a comment.
This revision is now accepted and ready to land.

One last nit, otherwise lgtm. But I'm not super familiar with this area, it'd be good to get another review from somebody more familiar.



================
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) {
----------------
zequanwu wrote:
> 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`.
Doesn't `DiagnoseUninitializedUse` say that we shouldn't warn at `int a = a`?


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1523
 
-  MappedType &getUses(const VarDecl *vd) {
+  MappedType &getUses(UsesMap &uses, const VarDecl *vd) {
     MappedType &V = uses[vd];
----------------
this is very confusing since it shadows the `uses` member variable from UninitValsDiagReporter


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

https://reviews.llvm.org/D79895