[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added a comment.

Thanks for the suggestion of splitting this patch to smaller ones.

I have one such smaller patch ready here: https://reviews.llvm.org/D156474.  It does make it easier in describing and discussing about the changes.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2430
+  // The union group over the ones in "Groups" that contain parameters of `D`:
+  llvm::SetVector<const VarDecl *>
+      GrpsUnionForParms; // these variables need to be fixed in one step
----------------
NoQ wrote:
> Why `SetVector`? Do we care about order? Maybe just `SmallSet`?
We'd like to keep the order of elements in this container deterministic.  Otherwise, the diagnostic message that lists those elements could be non-deterministic.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2171
+    if (VarGroupForVD.size() <= 1)
+      return "";
+
----------------
NoQ wrote:
> Does the empty string actually ever make sense for the caller? Maybe just assert that this never happens, and force the caller to check that?
The branch deals with the case where the group has only one member, which is the `VD`.  In that case, we do not list group mates of `VD` as there is none.   
This case is actually possible and conforms to the contract of this method.

The case where the group is empty is not suppose to happen.


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

https://reviews.llvm.org/D153059