awarzynski marked an inline comment as done. awarzynski added a comment. @clementval & @SouraVX - thank you for your comments! I will upload an updated version shortly.
================ Comment at: clang/include/clang/Driver/Options.td:4329 HelpText<"Generate machine code, but discard output">; -def emit_obj : Flag<["-"], "emit-obj">, - HelpText<"Emit native object files">; ---------------- SouraVX wrote: > Please correct if I've misunderstood this change ? > You're removing this option from here(`clang`) and defining again at line > 4631 as a common option to both `clang` `flang` ? > +1 to that. > However this seems out of the purview of this patch. Do you think having this > as a separate patch(with specific intent) would be good(For > tracking/isolating changes) ? > Or the least you can do is convey this intent in this patch Summary too. > I'm happy with either of those :) > You're removing this option from here(clang) and defining again at line 4631 > as a common option to both clang flang ? Correct :) > However this seems out of the purview of this patch. Not quite. Without this change you will get an error when using `-emit-obj`: ``` bin/flang-new -fc1 -emit-obj file.f90 error: unknown argument: '-emit-obj' ``` That's because it wouldn't be parsed here: https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/flang/lib/Frontend/CompilerInvocation.cpp#L166-L167 (note the definition of `includedFlagsBitmask`). I do agree that the Summary should be updated, thanks for pointing that out! ================ Comment at: clang/include/clang/Driver/Options.td:4629 -} // let Flags = [CC1Option] +} // let Flags = [CC1Option, NoDriverOption] + ---------------- clementval wrote: > Is it intended to modify the SYCL options? This is just updating the comment (so it's just an nfc, SYCL options are not affected). This closing `}` corresponds to the opening `{` specified here:: https://github.com/llvm/llvm-project/blob/eba09a2db9eab46832cb7ec7ef0d2c227747772a/clang/include/clang/Driver/Options.td#L4040. So, currently we have this: ``` let Flags = [CC1Option, NoDriverOption] in { // A lot of code HERE } // let Flags = [CC1Option] ``` Instead it should be: ``` let Flags = [CC1Option, NoDriverOption] in { // A lot of code HERE } // let Flags = [CC1Option, NoDriverOption] ``` I was mislead by this comment while working on this patch, so to me it seemed like a _related_ change :) But it isn't and should be extracted into a separate patch: https://github.com/llvm/llvm-project/commit/7f19712a6a9e65bdc9a9843ea488030bc12f3acc. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:104 // TODO: // case clang::driver::options::OPT_emit_obj: // case calng::driver::options::OPT_emit_llvm: ---------------- clementval wrote: > Should you remove this line since you have the case for it? Yes, thanks for pointing out! ================ Comment at: flang/test/Flang-Driver/code-gen.f90:15 + +! CHECK: code-generation is not available yet ---------------- SouraVX wrote: > Since it's an `error` NOT a string(or similar) generated, I would rather have > `ERROR` as a string check. This way it is self-evident for end reader. > Sort of: > ```! ERROR: code-generation is not available yet``` > Yeah, `ERROR` would be better. I wish that we had a prospect of Clang's `-verify`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93301/new/ https://reviews.llvm.org/D93301 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
