[PATCH] D77474: [analyzer][MallocChecker] Make NewDeleteLeaks depend on DynamicMemoryModeling rather than NewDelete

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:194
+               const ProgramPointTag *Tag = nullptr) {
+    // Say this 3 times fast.
+    State = State ? State : getState();
----------------
I like the joke, but this comment does not have any value, could you please remove?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:195
+    // Say this 3 times fast.
+    State = State ? State : getState();
+    addTransition(State, generateSink(State, getPredecessor()));
----------------
balazske wrote:
> ```
> if (!State)
>   State = getState();
> ```
> is better? (I put the same comment into some (other?) patch already but maybe it disappeared?)
+1 for balazske's suggestion.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1744-1745
 
+  // NOTE: There is a philosophical question to be answered when we detect a
+  // bug, but the checker under whose name we would emit the error is disabled.
+  // Generally speaking, the MallocChecker family is an integral part of the
----------------
This sentence is too bloated, I'd rather remove it.

AFAIU, the general rule of thumb is that if we have found a bug then we terminate further analysis, period. This is independent of whether we emit the warning or not, that actually depends on whether the corresponding subchecker is enabled or not.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2024
+  if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker]) {
+    C.addSink();
     return;
----------------
This seems to be inverse logic to me.
I'd expect that in a function called `Report...` we do stuff that is related to reporting only. That is why I think it would be better to have the condition and addSink before calling `Report...`. That way reporting and modeling would be even more separated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77474