[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I think it deserves an accept, however, as I don't agree with the rationale I'll let someone else for doing this.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h:111
 
-  /// \copydoc clang::ento::matchesAny(const CallEvent &, const CallDescription &)
+  /// \copydoc clang::ento::CallDescription::matchesAny(const CallEvent &, const CallDescription &)
   template <typename... Ts>
----------------
Szelethus wrote:
> steakhal wrote:
> > I think it's a free function. I know that copydoc did not work for this example.
> > Are you sure adding the `::CallDescription` fixes the doc comment?
> Yes! Its still in `CallDescription`s "namespace", as its defined in-class. I ran `doxygen-clang` and can confirm that this works, but only without line breaks (or at least I haven't figured out how to do it without it, not even with backslash)
Ah, thanks!


================
Comment at: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp:501
+//===----------------------------------------------------------------------===//
+// Testing through a checker interface.
+//
----------------
Szelethus wrote:
> steakhal wrote:
> > You could have define `ResultMap` ad a virtual base class, which would be implemented by two different classes. One of which would use the `asWritten` lookups, etc.
> > You could `make_unique` of the required one and inject it to the `Action`.
> > 
> > That way those tests would look just the same as the previous ones.
> My rationale was that reusing an already existing machinery which is already used in many of the static analyzer unit tests is far friendlier for beginners. I think fracturing the file specific stuff here would increase the barrier of entry for very little gain.
> 
> One of the things I like a lot on unit tests is that they demonstrate on a small scale a part of the analyzer's core machinery. I think dividing this up would go against that as well.
If you really think this way, you should port the existing tests of this file to the new format to get rid of the already existing "machinery".
I'm okay either way, but not with mixing the two approaches.


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

https://reviews.llvm.org/D119004