[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thanks for working on it.
Few comments inline:

1. For an out-of-tree build, I see `check-flang` target failing with

  /unittests/Frontend/CompilerInstanceTest.cpp:17:10: fatal error: filesystem: 
No such file or directory
   #include 
^~~~

I used gcc/g++ 7.5 version.
I haven't checked in-tree still, and others/bots might have checked it.

2. Either the documentation comments are wrong or code.

`README` mentions `DBUILD_FLANG_NEW_DRIVER` where as cmake ignores the flag for 
me.
Whereas, `CMakeLists.txt` mentions `FLANG_BUILD_NEW_DRIVER`.




Comment at: flang/include/flang/Frontend/TextDiagnosticBuffer.h:36
+  /// level and an index into the corresponding DiagList above.
+  std::vector> all_;
+

How about simplifying this with `using` keyword?



Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:37
+  /// Handle to the currently active text diagnostic emitter.
+  std::unique_ptr textDiag_;
+

Where is this used? I don't see any reference.



Comment at: flang/lib/Frontend/TextDiagnostic.cpp:12
+#include "llvm/Support/raw_ostream.h"
+#include 
+

Is this header used ?



Comment at: flang/lib/Frontend/TextDiagnostic.cpp:26
+static const enum llvm::raw_ostream::Colors savedColor =
+llvm::raw_ostream::SAVEDCOLOR;
+

Unless Flang is not changing the color scheme w.r.t clang, can't the code be 
shared between both projects?




Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:24
+
+TextDiagnosticPrinter::TextDiagnosticPrinter(
+raw_ostream &os, clang::DiagnosticOptions *diags)

A silly question from what I see usually in Flang coding style.
Why isn't it defined in header file?



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+  // this later as we print out the diagnostic to the terminal.
+  SmallString<100> outStr;
+  info.FormatDiagnostic(outStr);

kiranchandramohan wrote:
> 100? Will this contain path names by any chance?
Can we use at places where LLVM data structures are used explicit `llvm::` so 
an unknown user can easily identify where they come from?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added inline comments.



Comment at: flang/lib/Frontend/TextDiagnostic.cpp:26
+static const enum llvm::raw_ostream::Colors savedColor =
+llvm::raw_ostream::SAVEDCOLOR;
+

awarzynski wrote:
> sameeranjoshi wrote:
> > Unless Flang is not changing the color scheme w.r.t clang, can't the code 
> > be shared between both projects?
> > 
> That would be preferable, I agree. However, keep in mind that in Clang these 
> enums are defined in  llvm-project/clang/lib/Frontend/TextDiagnostic.cpp , 
> i.e. in `libclangFrontend`.  As suggested in [1], we shouldn't be re-using 
> anything from `libclangFrontend`. 
> 
> We could move these enums out of `libclangFrontend` to some other library. 
> That's not difficult and not that much work, but IMO that's a separate change 
> that should happen on top of this patch (i.e. once this patch is approved). 
> Also - where do we move them to? Currently there is no good candidate, but 
> once the refactoring proposed in [1] is complete, there should be a dedicated 
> library with infrastructure for the drivers to share.
> 
> I suggest that for now we just add a comment that recommends sharing these 
> enums with Clang in the future. This could happen once we start refactoring 
> Clang. In the meantime I'd like to keep the changes in Clang to the required 
> minimum. 
> 
> How does this sound? And does it make sense? 
> 
> [1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html
Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

In D87774#2292034 , @awarzynski wrote:

> @sameeranjoshi Apologies, I missed some of your comments.
>
> In D87774#2287927 , @sameeranjoshi 
> wrote:
>
>> Thanks for working on it.
>> Few comments inline:
>>
>> 1. For an out-of-tree build, I see `check-flang` target failing with
>>
>>   /unittests/Frontend/CompilerInstanceTest.cpp:17:10: fatal error: 
>> filesystem: No such file or directory
>>#include 
>> ^~~~
>>
>> I used gcc/g++ 7.5 version.
>> I haven't checked in-tree still, and others/bots might have checked it.
>
> I haven't been able to reproduce it, but note that `filesystem` is a C++17 
> header. AFAIK, in GCC-7 you have to use `` instead 
> of ``. This suggests README.md should be updated: 
> https://github.com/llvm/llvm-project/blob/master/flang/README.md#supported-c-compilers
>  (we probably need an RFC for this). Since this `#include` was introduced 
> elsewhere, I suggest that we move this discussion either to Slack or 
> flang-dev.

Do you know if there are any bots configured to handle out-of-tree changes?
That might be helpful to avoid configuration differences and test OFT patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added inline comments.



Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+  // this later as we print out the diagnostic to the terminal.
+  SmallString<100> outStr;
+  info.FormatDiagnostic(outStr);

awarzynski wrote:
> awarzynski wrote:
> > sameeranjoshi wrote:
> > > kiranchandramohan wrote:
> > > > 100? Will this contain path names by any chance?
> > > Can we use at places where LLVM data structures are used explicit 
> > > `llvm::` so an unknown user can easily identify where they come from?
> > If we do that, the code is likely to be bloated with `llvm::`, which IMO 
> > could be detrimental to readability in the long term. 
> > 
> > In Clang you will find a header file with all the forward declarations to 
> > avoid this: 
> > https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/LLVM.h.
> >  
> > 
> > I definitely want to make reading this code easier to users, but I also 
> > think that instead of `llvm::` one could use code-navigation tools (e.g. 
> > clangd). And forward declarations are fairly standard AFAIK. 
> > 
> > However, if you still think that `llvm::` would be better, I'm happy to 
> > update :)
> It shouldn't. Currently these diagnostics are only used to report errors from 
> the command line, so no paths are involved.
> 
> In Clang, similar class is used both for the driver diagnostics (i..e command 
> line errors - no source location) and frontend diagnostics (source code 
> errors - with source locations). However, AFAIK, this string is not used for 
> the source location (e.g. path).
> 
> If we do use this class for more Flang diagnostics, we shouldn't use `outStr` 
> for paths. This is just the message.
I think it's better to follow what `FIR`(fir-dev) and `llvm-project/flang` does.
They generally try to fully-qualify name of library and avoid using forward 
declarations.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thanks for the work.

A couple of comments on `clang/` related changes:
An `out-of-tree` build with this patch fails for me:
Here's what I did:
I initially used `ENABLE_PROJECTS="clang;mlir"` to build `llvm-project`, I 
didn't build `flang` during this run.

Then I passed following to the cmake when building `flang` as out of tree.

  cmake -G Ninja \
-DCMAKE_BUILD_TYPE=Release \
-DFLANG_ENABLE_WERROR=On \
-DCMAKE_CXX_STANDARD=17 \
-DLLVM_TARGETS_TO_BUILD=host \
-DLLVM_LIT_ARGS=-v \
-D LLVM_DIR=${LLVM_MLIR_CLANG_BUILD}/lib/cmake/llvm \
-D MLIR_DIR=${LLVM_MLIR_CLANG_BUILD}/lib/cmake/mlir \
-DBUILD_FLANG_NEW_DRIVER=ON \
../
  ninja -j16

where `LLVM_MLIR_CLANG_BUILD` points to initially built `llvm-project` .
I see below error message:

  ../lib/Frontend/CompilerInvocation.cpp: In static member function ‘static 
bool flang::CompilerInvocation::CreateFromArgs(flang::CompilerInvocation&, 
llvm::ArrayRef, clang::DiagnosticsEngine&)’:
  ../lib/Frontend/CompilerInvocation.cpp:85:67: error: ‘FlangOption’ is not a 
member of ‘clang::driver::options’
 clang::driver::options::CC1Option | 
clang::driver::options::FlangOption;
 ^~~
  ../lib/Frontend/CompilerInvocation.cpp:85:67: note: suggested alternative: 
‘LastOption’
 clang::driver::options::CC1Option | 
clang::driver::options::FlangOption;
 ^~~
 LastOption

Do you need to add `-DCLANG_DIR` flag, as there seem to be a dependency for 
this patch on clang as libraries?




Comment at: clang/include/clang/Driver/Options.td:60
+// FlangOption - This is considered a "core" Flang option, available in
+// flang mode
+def FlangOption : OptionFlag;

`nit:` a missing period.
// flang mode -> // flang mode.



Comment at: clang/include/clang/Driver/Options.td:2076
 def headerpad__max__install__names : Joined<["-"], 
"headerpad_max_install_names">;
-def help : Flag<["-", "--"], "help">, Flags<[CC1Option,CC1AsOption]>,
+def help : Flag<["-", "--"], "help">, 
Flags<[CC1Option,CC1AsOption,FlangOption]>,
   HelpText<"Display available options">;

`nit:` - A space before `FlangOption`
CC1AsOption,FlangOption -> CC1AsOption, FlangOption



Comment at: clang/include/clang/Driver/Options.td:3019
 // We give --version different semantics from -version.
-def _version : Flag<["--"], "version">, Flags<[CoreOption, CC1Option]>,
+def _version : Flag<["--"], "version">, Flags<[CoreOption, 
CC1Option,FlangOption]>,
   HelpText<"Print version information">;

Same as above.
nit: - A space before `FlangOption`



Comment at: clang/lib/Driver/Driver.cpp:1569
 
+  if (Mode == DriverMode::FlangMode) {
+ExcludedFlagsBitmask |= options::CLOption;

Can `IsFlangMode()` be used instead?



Comment at: clang/lib/Driver/Driver.cpp:1573
+ExcludedFlagsBitmask |= options::CC1Option;
+IncludedFlagsBitmask |= options::FlangOption;
+  } else

In `enum ClangFlags` 
inside File `clang/include/clang/Driver/Options.h`
there are various other options do they need to be considered?
If not, how are they handled?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi requested changes to this revision.
sameeranjoshi added a comment.

Thanks for working on it.
A few review comments/questions on changes in `flang` part from the patch.




Comment at: flang/include/flang/Frontend/CompilerInstance.h:93
+
+  static clang::IntrusiveRefCntPtr createDiagnostics(
+  clang::DiagnosticOptions *Opts,

The block of comments above make sense for this function and not the currently 
mentioned one.
Please interchange/replace the comments to later declared function.
Wrong comments above could reflect in `doxygen APIs`, misleading the reader of 
code.



Comment at: flang/include/flang/FrontendTool/Utils.h:1
+
+//===--- Utils.h - Misc utilities for the flang front-end *- 
C++-*-===//

`nit:`: blank line.



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:35
+  if (Flang->getFrontendOpts().ShowVersion) {
+llvm::cl::PrintVersionMessage();
+return true;

With 
```
clang --version
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 
1acf129bcf9a1b51e301a9fef151254ec4c7ec43)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin
```
whereas with 
```
./bin/flang-new --vesion
Flang experimental driver (flang-new)
```

I see both `clang` & `flang` call
`llvm::cl::PrintVersionMessage()` internally.

Is more information need to be registered in llvm(`llvm::cl`) for flang to give 
more detailed output or will that come later once we start adding more patch 
for driver?



Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:39
+
+  return true;
+}

The comment in header for `ExecuteCompilerInvocation` mentions,
```
/// \return - True on success.
bool ExecuteCompilerInvocation(CompilerInstance *Flang);
```

Do we need to have a `false` somewhere here?

I see 2 scenarios when `ExecuteCompilerInvocation` might fail (there could 
definitely be more) and there we need an indication of failure by returning 
`false`,
1. When there is no actual execution of compiler.
2. The compilation in not successful.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi accepted this revision.
sameeranjoshi added a comment.

Thank you for changes.
I was able to build successfully out-of-tree.
Please update the `README.md` with the necessary changes.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92854: [flang][driver] Add support for `-fsyntax-only`

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thanks for patch, a few queries below.

Are the tests in clang which are related to flang failing at 
https://reviews.llvm.org/B81474?




Comment at: flang/include/flang/Frontend/FrontendActions.h:29
 
+class SyntaxOnlyAction : public FrontendAction {
+  void ExecuteAction() override;

Do you think following a pattern here for mapping with `enum ActionKind` would 
be better?
something like `ParseSyntaxOnlyAction` ?



Comment at: flang/include/flang/Frontend/FrontendOptions.h:1
 //===- FrontendOptions.h *- C -*-===//
 //

C++ ?



Comment at: flang/lib/Frontend/FrontendActions.cpp:96
+  semantics.EmitMessages(ci.semaOutputStream());
+}

What happens in case one of the stages in the pipeline fails?
Where is the error recovery done for e.g if parsing fails ?



Comment at: flang/unittests/Frontend/PrintPreprocessedTest.cpp:121
+
+  // 4. Executre the ParseSyntaxOnly action
+  bool success = ExecuteCompilerInvocation(&compInst);

Execute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92854

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93027: [clang] Remove `-triple` from the invocations of `flang-new -fc1`

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi accepted this revision.
sameeranjoshi added a comment.
This revision is now accepted and ready to land.

Thanks for extracting it from D92854 .
I think the tests in D92854  were failing for 
the same reason.
LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93027

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92854: [flang][driver] Add support for `-fsyntax-only`

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi accepted this revision.
sameeranjoshi added a comment.
This revision is now accepted and ready to land.

Looks good from my end.
Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92854

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi accepted this revision.
sameeranjoshi added a comment.

Thanks for patch.
Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thank you, this patch looks easy to understand as it's clearly separated 
from(`D87989`) the infrastructure changes needed for frontend actions.
A few comments inline.




Comment at: flang/include/flang/Frontend/CompilerInstance.h:85
+  /// Return parsing to be used by Actions.
+  Fortran::parser::Parsing &GetParsing() const { return *parsing_; }
+

If I am correct this seems to be an accessor member function and it should 
follow point 3 from flang style guide  mentioned at 
https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#naming

I am not aware if driver related patches follow llvm-project style.



Comment at: flang/include/flang/Frontend/CompilerInvocation.h:12
 #include "flang/Frontend/FrontendOptions.h"
+#include "flang/Parser/parsing.h"
 #include "clang/Basic/Diagnostic.h"

`Nit:` I believe `clang-format` is missing.



Comment at: flang/lib/Frontend/CMakeLists.txt:17
   FortranParser
+  FortranSemantics
   clangBasic

I believe this patch is a parsing and preprocessing related patch, is this used 
somewhere or should this be removed for now?



Comment at: flang/lib/Frontend/CompilerInstance.cpp:118
+  // Fortran::parser::option
+  // Set defaults for Parser, as it use as flang options
+  // Default consistent with the temporary driver in f18/f18.cpp

May be you meant `as it is used`?



Comment at: flang/lib/Frontend/FrontendAction.cpp:8
 
//===--===//
-
 #include "flang/Frontend/FrontendAction.h"

undo



Comment at: flang/lib/Frontend/FrontendAction.cpp:57
+  // Read files, scan and run preprocessor
+  // Needed by all next fases of the frontend
+  GetCompilerInstance().GetParsing().Prescan(path, options);

`Nit:` Converts to `phases` in english according to my browser. :)



Comment at: flang/lib/Frontend/FrontendActions.cpp:62
+std::unique_ptr os;
+os = ci.CreateDefaultOutputFile(
+/*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName());

https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#c-language
point 4 
Can you use `braced initializers` ?

A more better version I think would be to simplify this to 
```
if (auto os{ci.CreateDefaultOutputFile(/*Binary=*/true, 
/*InFile=*/GetCurrentFileOrBufferName())}){
(*os) << out.str();
} else {
  llvm::errs() << "Unable to create the output file\n";
  return;
}
```



Comment at: flang/test/Frontend/Inputs/hello-world.c:1
+a #include int main() {
+  // printf() displays the string inside quotation

a - ?


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

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

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

A few `nits:` and mostly style comments inline:




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:
> > `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



Comment at: flang/include/flang/Frontend/FrontendActions.h:9
 
 #ifndef LLVM_FLANG_FRONTEND_FRONTENDACTIONS_H
 #define LLVM_FLANG_FRONTEND_FRONTENDACTIONS_H

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


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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93401: [flang][driver] Add support for `-D`, `-U`

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added inline comments.



Comment at: flang/test/Flang-Driver/driver-help.f90:22
 ! HELP-NEXT: -###   Print (but do not run) the commands to run 
for this compilation
+! HELP-NEXT: -D = Define  to  (or 1 if  
omitted)
 ! HELP-NEXT: -E Only run the preprocessor

I see below crash report, when omitting the  but not omitting the `=` 
symbol.
Not sure if that's correct way of running hence instead of filing bug reporting 
here.

```
./bin/flang-new -E -DX= test.f90
```

```
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace.
Stack dump:
0.  Program arguments: 
/home/amd/f18_git/final_test/driver_build/bin/flang-new -fc1 -E -D X= -o - 
test.f90
 #0 0x7f26185d0bc1 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/libLLVMSupport.so.12git+0x1a4bc1)
 #1 0x7f26185ce9a4 llvm::sys::RunSignalHandlers() 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/libLLVMSupport.so.12git+0x1a29a4)
 #2 0x7f26185ceb10 SignalHandler(int) 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/libLLVMSupport.so.12git+0x1a2b10)
 #3 0x7f2617749470 (/lib/x86_64-linux-gnu/libc.so.6+0x46470)
 #4 0x7f2617443498 
Fortran::parser::OffsetToProvenanceMappings::Map(unsigned long) const 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/../lib/libFortranParser.so.12git+0x571498)
 #5 0x7f261744adf2 Fortran::parser::TokenSequence::GetProvenanceRange() 
const 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/../lib/libFortranParser.so.12git+0x578df2)
 #6 0x7f26173dd334 
Fortran::parser::Preprocessor::MacroReplacement(Fortran::parser::TokenSequence 
const&, Fortran::parser::Prescanner&) (.localalias) 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/../lib/libFortranParser.so.12git+0x50b334)
 #7 0x7f26173e9dc1 Fortran::parser::Prescanner::Statement() (.localalias) 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/../lib/libFortranParser.so.12git+0x517dc1)
 #8 0x7f26173ea888 
Fortran::parser::Prescanner::Prescan(Fortran::common::Interval)
 (.localalias) 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/../lib/libFortranParser.so.12git+0x51)
 #9 0x7f26173d480b 
Fortran::parser::Parsing::Prescan(std::__cxx11::basic_string, std::allocator > const&, 
Fortran::parser::Options) 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/../lib/libFortranParser.so.12git+0x50280b)
#10 0x7f2618424ddd Fortran::frontend::FrontendAction::Execute() 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/libflangFrontend.so.12git+0x)
#11 0x7f261841fa5e 
Fortran::frontend::CompilerInstance::ExecuteAction(Fortran::frontend::FrontendAction&)
 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/libflangFrontend.so.12git+0x8a5e)
#12 0x7f26184132fc 
Fortran::frontend::ExecuteCompilerInvocation(Fortran::frontend::CompilerInstance*)
 
(/home/amd/f18_git/final_test/driver_build/bin/../lib/libflangFrontendTool.so.12git+0x12fc)
#13 0x55bad4081c20 fc1_main(llvm::ArrayRef, char const*) 
(/home/amd/f18_git/final_test/driver_build/bin/flang-new+0x3c20)
#14 0x55bad4080e59 main 
(/home/amd/f18_git/final_test/driver_build/bin/flang-new+0x2e59)
#15 0x7f261772a1e3 __libc_start_main 
/build/glibc-5mDdLG/glibc-2.30/csu/../csu/libc-start.c:342:3
#16 0x55bad4080eae _start 
(/home/amd/f18_git/final_test/driver_build/bin/flang-new+0x2eae)
flang-new: error: unable to execute command: Segmentation fault (core dumped)
flang-new: error: flang frontend command failed due to signal (use -v to see 
invocation)
flang-new version 12.0.0 (https://github.com/llvm/llvm-project.git 
2f9cb090cc6db1be5bf524eb0a32537503b3e786)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/amd/f18_git/final_test/driver_build/bin
flang-new: note: diagnostic msg: Error generating preprocessed source(s) - no 
preprocessable inputs.

```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93401

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93453: [flang][driver] Add support for `-I`

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added inline comments.



Comment at: flang/test/Flang-Driver/include-header.f90:38
+!-
+! EXPECTED OUTPUT FOR /Inputs/ FOLDER SPECIFIED FIRST
+!-

Nit - Shouldn't this be `Inputs/` starting with a `/` changes the meaning(at 
least on Linux systems) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93453

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits