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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

OOo down to ±300, I love this!! I'll take a closer look tomorrow.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1891-1892
+  for (unsigned i = 0; i < NumParms; i++) {
+    if (!ParmsMask[i])
+      continue;
+
----------------
Speaking of [[ https://reviews.llvm.org/D156762#inline-1517322 |my comment on the other patch]].

Can we simply ask `Strategy` about the strategy for `FD->getParamDecl(i)` instead of passing a mask?

Eventually we'll have to do that anyway, given how different parameters can be assigned different strategies.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1935
         // print parameter name if provided:
-        if (IdentifierInfo * II = Parm->getIdentifier())
-          SS << " " << II->getName().str();
-      } else if (auto ParmTypeText =
-                     getRangeText(Parm->getSourceRange(), SM, LangOpts)) {
+        if (auto II = Parm->getIdentifier())
+          SS << ' ' << II->getName().str();
----------------
Arguably not a great use of `auto`, because the return type isn't literally `Identifier *`.


================
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
----------------
ziqingluo-90 wrote:
> 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.
Ooo right gotcha!


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2171
+    if (VarGroupForVD.size() <= 1)
+      return "";
+
----------------
ziqingluo-90 wrote:
> 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.
Gotcha, nice!


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

https://reviews.llvm.org/D153059