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