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

dexonsmith added a subscriber: aprantl. dexonsmith added a comment. In D94472#3052604 , @dexonsmith wrote: > Looking at `CommandLineArgs`, and its eventual forward to `MCTargetInfo`, the > existing design looks like a reference leak, which would also bloc

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

dexonsmith added a comment. Looking at `CommandLineArgs`, and its eventual forward to `MCTargetInfo`, the existing design looks like a reference leak, which would also block freeing Clang memory when LLVM is still running. Given that 89ea0b05207d45c145fb525df554b3b986ae379b

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

saudi added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677 + SwapOpts(Res); + bool Success2 = Parse(Res, GeneratedArgs1, Diags); + jansvoboda11 wrote: > saudi wrote: > > jansvoboda11 wrote: > > > saudi wrote: > > > > Hello, > > > >

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

jansvoboda11 added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677 + SwapOpts(Res); + bool Success2 = Parse(Res, GeneratedArgs1, Diags); + saudi wrote: > jansvoboda11 wrote: > > saudi wrote: > > > Hello, > > > > > > I encountered cra

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

saudi added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677 + SwapOpts(Res); + bool Success2 = Parse(Res, GeneratedArgs1, Diags); + jansvoboda11 wrote: > saudi wrote: > > Hello, > > > > I encountered crashes on Windows targets, relat

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

jansvoboda11 added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677 + SwapOpts(Res); + bool Success2 = Parse(Res, GeneratedArgs1, Diags); + saudi wrote: > Hello, > > I encountered crashes on Windows targets, related to this line, when

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

saudi added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:677 + SwapOpts(Res); + bool Success2 = Parse(Res, GeneratedArgs1, Diags); + Hello, I encountered crashes on Windows targets, related to this line, when rebasing https://reviews

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG225ccf0c50a8: [clang][cli] Command line round-trip for HeaderSearch options (authored by jansvoboda11). Changed prior to commit: https://reviews.l

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

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D94472#2539685 , @jansvoboda11 wrote: > I've changed this patch so that the errors and debugging output goes to > `DiagnosticsEngine`. > >

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

jansvoboda11 added a comment. I've changed this patch so that the errors and debugging output goes to `DiagnosticsEngine`. Also, I switched back to the original solution, where we always round-trip when Clang is built certain way (this time not when CMake was configured with `-DLLVM_ENABLE_ASS

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

jansvoboda11 updated this revision to Diff 321137. jansvoboda11 added a comment. Herald added a subscriber: mgorny. Don't round-trip when the driver says so. Round-trip all the time when built to do so. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

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

jansvoboda11 updated this revision to Diff 320342. jansvoboda11 added a comment. Fix formatting and comment wording Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94472/new/ https://reviews.llvm.org/D94472 Files: clang/include/clang/Basic/Diagnos

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

jansvoboda11 added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:684-685 + if (!GeneratedStringsEqual) +llvm::report_fatal_error("Mismatch between arguments generated during " + "round-trip"); + dexonsmith

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

jansvoboda11 updated this revision to Diff 320315. jansvoboda11 added a comment. Create proper diagnostics, add remark tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94472/new/ https://reviews.llvm.org/D94472 Files: clang/include/clang/Basi

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

dexonsmith added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:684-685 + if (!GeneratedStringsEqual) +llvm::report_fatal_error("Mismatch between arguments generated during " + "round-trip"); + jansvoboda11

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

jansvoboda11 added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:584-589 +static bool Equal(const ArgStringList &A, const ArgStringList &B) { + auto StrEq = [](const char *AElem, const char *BElem) { +return StringRef(AElem) == StringRef(BElem); + }

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

jansvoboda11 updated this revision to Diff 320115. jansvoboda11 added a comment. Improve error handling Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94472/new/ https://reviews.llvm.org/D94472 Files: clang/include/clang/Driver/Options.td clang

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

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. This is getting close! A few more comments inline. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:584-589 +static bool Equal(const ArgStringList &A,

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

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

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

jansvoboda11 updated this revision to Diff 319868. jansvoboda11 added a comment. Ensure Diags get used only once, add disabling flag, clean up Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94472/new/ https://reviews.llvm.org/D94472 Files: clang/

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

dexonsmith 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 ro

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

jansvoboda11 added a comment. In D94472#2523053 , @Bigcheese wrote: > The only issue I have with this if always parsing twice has a noticeable > performance impact for any users of Clang. Can we measure the impact? If it's > small (< 100ms ?) then that's