NoQ added a comment.
Herald added subscribers: martong, steakhal.

I didn't use it yet but i like it. The code is easy to understand, so i'll be 
happy to maintain it as it lands.

The example checker is nice and i like how you made sure it demonstrates modern 
API usage. I'm a bit worried that it may quickly get out of date as we update 
our APIs, because this happens like ~multiple times a year, much more often 
than in clang-tidy. Is there a way to write a test that tests that the 
generated checker //compiles//? I don't have any specific ideas but it would 
have been awesome.



================
Comment at: clang/utils/analyzer/add-new-checker.py:79-81
+        data = subprocess.check_output(['llvm-tblgen', '-dump-json',
+                                        checkers_path,
+                                        '-I=' + checkers_include_path])
----------------
`llvm-tblgen` needs to be in the `PATH`, right? What if LLVM isn't installed on 
the host system or has the wrong version?

We might have to either tell the user to specify it explicitly in the 
invocation (and then update the LIT substitution to explicitly use the right 
binary), or, ideally, try to get the script installed into the build directory 
so that it could find the freshly built `llvm-tblgen` binary relative to itself.


================
Comment at: clang/utils/analyzer/add-new-checker.py:573
+
+REGISTER_MAP_WITH_PROGRAMSTATE(AwesomeMap, const MemRegion *, bool)
+
----------------
I think we should say explicitly that these are examples, not required code, 
because a lot of this may look like magic to a beginner.

I.e., "Here's how you define a state trait", etc.


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

https://reviews.llvm.org/D73521



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to