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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
