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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits