RedDocMD marked 5 inline comments as done. RedDocMD added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:435 + void markNotInteresting(SymbolRef sym); + ---------------- xazax.hun wrote: > Bikeshedding: I wonder if we prefer `Uninteresting` to `NotInteresting`. Or > alternatively, if we want to emphasize this is only the lack of > interestingness, we could name it `removeInterestingness`. I do not have > strong feelings about any of the options, I was just wondering if any of you > has a preference. I was actually planning to use something like `unmarkInteresting`, but that is grammatically incorrect. `removeInterestingness` is fair enough, but a mouthful I think. As for `Uninteresting`, I think we are better off avoiding clever uses of English and stick with simple, programmer jargon. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:191 + // Check the first arg, if it is of std::unique_ptr type. + assert(Call.getNumArgs() == 2 && "std::swap should have two arguments"); + const Expr *FirstArg = Call.getArgExpr(0); ---------------- xazax.hun wrote: > I wonder about the value of this assertion. Shouldn`t > `Call.isCalled(StdSwapCall)` already validate the number of arguments? About as valuable as any assertion is ;) My reasoning is that since we are calling `Call.getArgSval(0)` and `Call.getArgSVal(1)`, we had better assert that there are two args. As a side-note, I had a bug in my `CallDescriptor`, which was caught by this assertion ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:196-202 + const MemRegion *FirstArgThisRegion = Call.getArgSVal(0).getAsRegion(); + if (!FirstArgThisRegion) + return false; + const MemRegion *SecondArgThisRegion = Call.getArgSVal(1).getAsRegion(); + if (!SecondArgThisRegion) + return false; + ---------------- vsavchenko wrote: > I guess `handleSwap` can take `SVal`s instead of `MemRegion` and we can > mostly cut on this boilerplate as well. > ``` > return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C); > ``` > and > ``` > return handleSwap(State, IC->getCXXThisVal(), Call.getArgSVal(0), C); > ``` Yup, okay. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:458-467 + if (BR.isInteresting(FirstThisRegion) && + !BR.isInteresting(SecondThisRegion)) { + BR.markInteresting(SecondThisRegion); + BR.markNotInteresting(FirstThisRegion); + } + if (BR.isInteresting(SecondThisRegion) && + !BR.isInteresting(FirstThisRegion)) { ---------------- xazax.hun wrote: > vsavchenko wrote: > > nit: these two pieces of code are very much the same > I guess we could make `markInteresting` take an optional bool and swap the > interestingness unconditionally. Then we would need to rename it to something like `setInterestingness`. And renaming such a widely used function would be a real pain. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:458-467 + if (BR.isInteresting(FirstThisRegion) && + !BR.isInteresting(SecondThisRegion)) { + BR.markInteresting(SecondThisRegion); + BR.markNotInteresting(FirstThisRegion); + } + if (BR.isInteresting(SecondThisRegion) && + !BR.isInteresting(FirstThisRegion)) { ---------------- RedDocMD wrote: > xazax.hun wrote: > > vsavchenko wrote: > > > nit: these two pieces of code are very much the same > > I guess we could make `markInteresting` take an optional bool and swap the > > interestingness unconditionally. > Then we would need to rename it to something like `setInterestingness`. > And renaming such a widely used function would be a real pain. Yes, but I don't think refactoring them into a lambda (within a lambda) would be that nice either. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104300/new/ https://reviews.llvm.org/D104300 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits