[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

Fri Apr 29 08:46:16 PDT 2022

mantognini added a comment.

In D124462#3480348 <https://reviews.llvm.org/D124462#3480348>, @aaron.ballman wrote:

> In D124462#3480219 <https://reviews.llvm.org/D124462#3480219>, @steakhal wrote:
>
>> Thanks for the stats.
>> @aaron.ballman WDYT, where should we put the `LLVM_DUMP_METHOD` ?
>
> The documentation for the attribute says function definitions, but I don't think it matters in terms of the semantics of the attributes. I'd probably put it on the definition in this case because the goal is to keep the function around at runtime but it doesn't impact the interface of the call or how users would use it at compile time.
>
> I noticed that we seem to be pretty bad about this advice however:
>
>   /// Note that you should also surround dump() functions with
>   /// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always
>   /// get stripped in release builds.

I don't have a strong opinion on where `LLVM_DUMP_METHOD` should be and whether this means writing it twice as I can see arguments for both approaches.

In addition to not ifdef-out those functions, I note that many `dump` or `dumpToStream` functions in `Analysis` (and LLVM in general) don't have the `LLVM_DUMP_METHOD` macro at all.

So I'm tempted to say the overall situation should be improved in a separate effort, and this patch can focus only on the orthogonal linking issue. WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124462