sameeranjoshi added a comment. Looks good, I would wait for a couple of more days for someone to review from community may be from Nvidia's side if someone would verify the initial design. Thank you.
================ Comment at: flang/include/flang/Frontend/CompilerInvocation.h:12 #include "flang/Frontend/FrontendOptions.h" +#include "flang/Parser/parsing.h" #include "clang/Basic/Diagnostic.h" ---------------- awarzynski wrote: > sameeranjoshi wrote: > > awarzynski wrote: > > > sameeranjoshi wrote: > > > > `Nit:` I believe `clang-format` is missing. > > > I did apply it and this line looks OK to me - what's missing? > > See point 2[1] below the code block. > > Ideally clang comes alphabetically first. > > > > [1] > > https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#files > Thanks for pointing this out! I appreciate that Flang has its own coding > style, but I think that in terms of the order of include files, the rules > from LLVM's coding standard [1] and Flang's are actually similar. From [1]: > > We prefer these #includes to be listed in this order: > > > > Main Module Header > > Local/Private Headers > > LLVM project/subproject headers (clang/..., lldb/..., llvm/..., etc) > > System #includes > > and each category should be sorted lexicographically by the full path. > > This is then further clarified as: > > > LLVM project and subproject headers should be grouped from most specific to > > least specific, for the same reasons described above. For example, LLDB > > depends on both clang and LLVM, and clang depends on LLVM. So an LLDB > > source file should include lldb headers first, followed by clang headers, > > followed by llvm headers, to reduce the possibility (for example) of an > > LLDB header accidentally picking up a missing include due to the previous > > inclusion of that header in the main source file or some earlier header > > file. clang should similarly include its own headers before including llvm > > headers. This rule applies to all LLVM subprojects. > > That's the rule that I applied here. This is consistent with the current > .clang-format configuration for Flang: > https://github.com/llvm/llvm-project/blob/d26dd743084a886382204ede5eeed146cd29fcd6/flang/.clang-format#L10-L18. > This is also why clang-format believes this is correct. > > In other words, there are two rules: > * project specific header files first (i.e. header files from Flang first) > * then, use alphabetic order. > > I do feel that both [1] and [2] leave room for interpretation. If we are > still in disagreement, we can follow-up on flang-dev or in one of the weekly > calls. I'm keen to get this right! > > [1] https://llvm.org/docs/CodingStandards.html#include-style > [2] https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md Thanks for details. I was unaware of the fact that local project headers should come first, I was assuming everything is lexicographically sorted. ================ Comment at: flang/include/flang/Frontend/FrontendActions.h:9 #ifndef LLVM_FLANG_FRONTEND_FRONTENDACTIONS_H #define LLVM_FLANG_FRONTEND_FRONTENDACTIONS_H ---------------- awarzynski wrote: > sameeranjoshi wrote: > > Why is this not following the style guide mentioned at [1] point 2. > > > > [1] > > https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#files > I'm guessing that you are referring to this rule specifically: > ``` > Header files should be idempotent. Use the usual technique: > > #ifndef FORTRAN_header_H_ > #define FORTRAN_header_H_ > // code > #endif // FORTRAN_header_H_ > ``` > > To me this rule reads: "use hash guards". The rule doesn't mention what the > format of the hash guard should be. However, LLVM's style guide does: > https://llvm.org/docs/CodingStandards.html#header-guard. We've adopted it > based on the following rule from Flang's style guide: > ``` > Otherwise, where LLVM's C++ style guide is clear on usage, follow it. > ``` > > The format of the hash guards hasn't been contested in any of the previous > reviews, so we assume that the format is fine. > > And if this is not about the format of the hash guard - please clarify :) I reverted back seeing the clang-tidy warnings and no `FORTRAN` prefix but a `LLVM_FLANG` prefix. If it was approved in previous reviews, I think that OK to keep it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88381/new/ https://reviews.llvm.org/D88381 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
