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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
