[PATCH] D67253: clang-misexpect: a standalone tool for verifying the use of __builtin_expect with PGO data

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Re concurrency - you had standalone `LLVMContext` for each thread, right?

In D67253#1670667 <https://reviews.llvm.org/D67253#1670667>, @paulkirth wrote:

> In D67253#1670569 <https://reviews.llvm.org/D67253#1670569>, @lebedev.ri wrote:
>
> > Layering feels weird here.
> >  Can this be implemented as/limited to just a
> >  `run-clang-misexpect.py` wrapper over clang itself?
> >  That would be best IMHO.
>
>
> I discussed the concurrency issue with some folks who work on libTooling, notably Sam McCall & Dmitri Gribenko. This was the approach they suggested I follow. LibTooling also provides some nice mechanisms for curating compiler options, which is possible, but less than ideal to reimplement in a python script. There are probably more benefits that escape me at the moment, but that was the first one I thought of.
>
> Out of curiosity, if the concurrency issue was fixed in the compiler and the python script was removed/deprecated, would you still feel the same way?


I was actually talking/thinking about the opposite direction, only having the .py wrapper, no standalone tool;
so the opposite solution (no .py, only the tool) is tangential/has different direction.



================
Comment at: clang-tools-extra/docs/clang-misexpect.rst:1-3
+===================
+Clang-Misexpect
+===================
----------------
I was just thinking that i forgot to mention docs in the original review, so thanks.

That being said, i do wonder whether this docs should be reworked
into two separate pages as it is a //bit// misleading right now.
The bulk of this should talk about
clang's `-Wmisexpect`/llvm's `-pgo-warn-misexpect`.

Because right now it seems as-if one has to use the tool,
and as it was brought up in the lists multiple times,
[depending on the exact workflow] that is more involved
than simply passing one more warning flag to the compiler.

(Yes, after specifically looking for it, i can see that
there are mentions/hints that the tool isn't required,
but it's a bit too subtle.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67253