[PATCH] D84494: [Analyzer] Use of BugType in DereferenceChecker (NFC).

Tue Jul 28 19:04:19 PDT 2020

NoQ added a comment.

> `BuiltinBug` is deprecated and we should be using `BugType` anyway

Like, i've never heard of `BuiltinBug` being deprecated but i'm pretty happy that it gets removed :)

================
Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:215
   // The explicit NULL case.
+  // We know that 'location' cannot be non-null.  This is what
+  // we call an "explicit" null dereference.
----------------
vsavchenko wrote:
> Maybe "is definitely null" here? Otherwise it can be pretty confusing with a double negation.
Also this entire comment should be inside the if-statement, on the same level as the next comment.

================
Comment at: clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist:270
    <key>description</key><string>Dereference of null pointer (loaded from variable 'x')</string>
-   <key>category</key><string>Logic error</string>
+   <key>category</key><string>Memory error</string>
    <key>type</key><string>Dereference of null pointer</string>
----------------
balazske wrote:
> vsavchenko wrote:
> > I might've missed some discussions on that matter, so please correct me on that.
> > In my opinion, null pointer dereference is not a memory error.  Null address is not a correct memory and never was, so it is a logic error that a special value is being interpreted as the pointer to something.
> I can not decide which is better, "logic error" can be said for almost every possible bug. Here the problem is at least related to memory handling. The reference of undefined pointer value is "logic error" too (it is known that the value was not initialized) but a memory error (try to access invalid or valid but wrong address). Probably "pointer handling error" is better?
> (One value for bug category is not a good approach, it is never possible to classify a bug to exactly one.)
Maybe let's not change it then, if there are no clear pros or cons? Users already got used to it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84494