[PATCH] D88381: [Flang][Driver] Add PrintPreprocessed FrontendAction

[sameeran joshi via Phabricator via cfe-commits]

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