[PATCH] D81995: [xray] Option to omit the function index

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:114
+///< Set with -fno-xray-function-index to omit the index section.
+CODEGENOPT(XRayOmitFunctionIndex , 1, 0)
+
----------------
ianlevesque wrote:
> MaskRay wrote:
> > Nit: a variable name with a positive meaning may be easier to understand.
> I had the entire patch that way at first, making the index something you would enable (defaulting to true) but it was much worse.
The variable naming should be easily inferrable from the option name. The option does not say `omit`. Doesn't `XRayFunctionIndex` work? How can it be much worse?


================
Comment at: clang/include/clang/Driver/Options.td:1284
 
+def fxray_function_index : Flag<["-"], "fxray-function-index">,
+  Group<f_Group>, Flags<[CC1Option]>;
----------------
This should use OptOutFFlag. I fixed it.


================
Comment at: clang/test/Driver/XRay/xray-function-index-flags.cpp:6
+// RUN: %clang -fxray-instrument -fxray-function-index -target x86_64-linux- -### \
+// RUN:     -x c++ -std=c++11 -emit-llvm -c -o - %s 2>&1 \
+// RUN:     | FileCheck %s
----------------
MaskRay wrote:
> If c++11 is not relevant, drop it.
Already dropped and cleaned a bit.


================
Comment at: clang/test/Driver/XRay/xray-function-index-flags.cpp:20
+//
+// REQUIRES: x86_64 || x86_64h
----------------
ianlevesque wrote:
> MaskRay wrote:
> > I know some tests may be inconsistent but the prevailing style is to place `REQUIRES:` at the top.
> I already committed this, but I can do a quick follow up for these nits.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81995