[PATCH] D94472: [clang][cli] Command line round-trip for HeaderSearch options

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:593-594
 
+def round_trip_args : Flag<["-"], "round-trip-args">, Flags<[CC1Option, NoDriverOption]>,
+  HelpText<"Performs 'parse-generate-parse' round-trip of command line arguments.">;
+def round_trip_args_debug : Flag<["-"], "round-trip-args-debug">, Flags<[CC1Option, NoDriverOption]>,
----------------
dexonsmith wrote:
> Can we make this `=true` vs. `=false`, or add a `-no-round-trip-args`? That will allow:
> ```
> % clang some args -Xclang -no-round-trip-args
> ```
> to disable it in asserts builds; much better than:
> ```
> % clang some args -###
> # Search for -round-trip-args
> % clang -cc1 copy-paste-args-before copy-past-args-after
> ```
Done.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:531
+  Option Opt = getDriverOptTable().getOption(OptSpecifier);
+  denormalizeSimpleFlag(Args, SA(Opt.getPrefix() + Opt.getName()), SA,
+                        Option::OptionClass::FlagClass, 0);
----------------
dexonsmith wrote:
> Hmm, I thought we'd found a way to avoid allocating the prefixed name, by adding it to the options table. This unfortunate, but I guess you can fix it another time.
> 
> 
We can avoid the allocation in the generating macro. I'll come back to this later and figure out a way to do the same here.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:540
+  Option Opt = getDriverOptTable().getOption(OptSpecifier);
+  denormalizeString(Args, SA(Opt.getPrefix() + Opt.getName()), SA,
+                    Opt.getKind(), 0, Value);
----------------
dexonsmith wrote:
> Does this allocated string get used, or does `"=some-value"` get added on?
Both. It depends on the kind/class (Joined/Separate/...) of the option.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3211-3214
+  auto ResetHS = [](CompilerInvocation &Res) {
+    auto EmptyHeaderSearchOpts = std::make_shared<HeaderSearchOptions>();
+    Res.HeaderSearchOpts.swap(EmptyHeaderSearchOpts);
+  };
----------------
dexonsmith wrote:
> I'm not sure this works correctly. Callers may depend on the identity of the `HeaderSearchOptions`. It's a shared pointer that could have other references. This will only update / reset one reference.
Resolved by `SwapOpts`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94472