[PATCH] D99292: [flang][driver] Add support for `-cpp/-nocpp`
tskeith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4302 +def cpp : Flag<["-"], "cpp">, Group, + HelpText<"Always add standard macro predefinitions">; +def nocpp : Flag<["-"], "nocpp">, Group, This option affects command line macro definitions too. So maybe something like: `Enable predefined and command line preprocessor macros` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99292/new/ https://reviews.llvm.org/D99292 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98191: [flang][driver] Add support for `-fdebug-dump-symbols-sources`
tskeith added a comment. > Would this option be used to extract debug/code-navigation info? Yes, it's something related to mapping between symbols and source locations. > Is there an equivalent in `clang` or `gfortran`? Not that I know of. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98191/new/ https://reviews.llvm.org/D98191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97119: [flang][driver] Add options for -std=f2018
tskeith added inline comments. Comment at: flang/test/Driver/std2018.f90:9 +! RUN: %flang_fc1 -pedantic %s 2>&1 | FileCheck %s --check-prefix=GIVEN +! RUN: not %flang_fc1 -std=90 %s 2>&1 | FileCheck %s --check-prefix=WRONG + You need to make sure these work with f18 or indicate the test requires the new driver. f18 doesn't currently support `-std=f2018` and doesn't complain about unrecognized options. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97119/new/ https://reviews.llvm.org/D97119 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97080: [flang][driver] Add -fintrinsic-modules-path option
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:293
+ driverPath = driverPath.substr(0, driverPath.size() - 9);
+ return driverPath.append("/../tools/flang/include/flang/");
+}
tskeith wrote:
> arnamoy10 wrote:
> > tskeith wrote:
> > > Does this work for an install? I think there the path would be
> > > `include/flang` rather than `tools/flang/include/flang`.
> > You are probably right, I am giving the path w.r.t a build. Can we make
> > the assumption that there should be always an install? Or do we determine
> > if we flang-new is coming from build or install (by checking if a module
> > file is present through ls) and then set the path accordingly?
> I think it should work in a build or an install. The `flang/tools/f18/flang`
> script checks for `include/flang` first and if that doesn't exist it uses
> `tools/flang/include/flang` so you could do the same. It would be good if we
> could fix the build or install so that the location is consistent.
I've created D98522 to make the relative path be `include/flang` in both build
and install. If no one objects, that should make it so you don't need
conditional code here, just use `"/../include/flang"`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97080/new/
https://reviews.llvm.org/D97080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97080: [flang][driver] Add -fintrinsic-modules-path option
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:293
+ driverPath = driverPath.substr(0, driverPath.size() - 9);
+ return driverPath.append("/../tools/flang/include/flang/");
+}
arnamoy10 wrote:
> tskeith wrote:
> > Does this work for an install? I think there the path would be
> > `include/flang` rather than `tools/flang/include/flang`.
> You are probably right, I am giving the path w.r.t a build. Can we make the
> assumption that there should be always an install? Or do we determine if we
> flang-new is coming from build or install (by checking if a module file is
> present through ls) and then set the path accordingly?
I think it should work in a build or an install. The `flang/tools/f18/flang`
script checks for `include/flang` first and if that doesn't exist it uses
`tools/flang/include/flang` so you could do the same. It would be good if we
could fix the build or install so that the location is consistent.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97080/new/
https://reviews.llvm.org/D97080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97080: [flang][driver] Add -fintrinsic-modules-path option
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:293
+ driverPath = driverPath.substr(0, driverPath.size() - 9);
+ return driverPath.append("/../tools/flang/include/flang/");
+}
Does this work for an install? I think there the path would be `include/flang`
rather than `tools/flang/include/flang`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97080/new/
https://reviews.llvm.org/D97080
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98191: [flang][driver] Add support for `-fdebug-dump-symbols-sources`
tskeith added a comment. `-fget-symbols-sources` is not a debug option, it's intended for integrating with IDEs like vscode. So I think the original name is better. Unlike the "dump" options it actually is an action and not something that is intended to produce debug output on the way to doing something else. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98191/new/ https://reviews.llvm.org/D98191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96875: [flang][driver] Add -fdebug-module-writer option
tskeith accepted this revision. tskeith added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96875/new/ https://reviews.llvm.org/D96875 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D97119: [flang][driver] Add options for -std=2018
tskeith added a comment.
Please make sure the test works with f18 also.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:294
+// We only allow 2018 as the given standard
+if (standard.equals("2018")) {
+ res.SetStandard();
This should be "f2018", not "2018".
Comment at: flang/test/Flang-Driver/std2018.f90:18
+!-
+! GIVEN: A DO loop should terminate with an END DO or CONTINUE
+
Can you verify this message is not produced when there is no -std option?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97119/new/
https://reviews.llvm.org/D97119
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments.
Comment at: flang/test/Flang-Driver/fdefault.f90:4
+
+! REQUIRES: new-flang-driver
+
awarzynski wrote:
> tskeith wrote:
> > Can't this work with the f18 driver too? That's the best way to ensure they
> > are consistent.
> I think that this is a good idea, but there are two things that need to be
> addressed first:
>
> **OPTION NAMES**
>
> `f18` uses `-module` rather than `-module-dir`. I suggest adding an alias,
> `-module-dir`, to `f18` (e.g. similarly to how `-fparse-only` and
> `-fsyntax-only` are handled)
>
> **IMPLEMENTATION**
> If I understand correctly, the current implementation of `-fdefault-real-8`
> in `f18` is incomplete, see [[
> https://github.com/llvm/llvm-project/blob/b6db47d7e044730dc3c9b35dae6697eee0885dbf/flang/tools/f18/f18.cpp#L568-L569
> | here ]]:
> ```
> } else if (arg == "-r8" || arg == "-fdefault-real-8") {
> defaultKinds.set_defaultRealKind(8);
> ```
> I think that there should be `defaultKinds.set_defaultRealKind(16);` as well.
>
> @tskeith could you confirm?
> I think that this is a good idea, but there are two things that need to be
> addressed first:
>
> **OPTION NAMES**
>
> `f18` uses `-module` rather than `-module-dir`. I suggest adding an alias,
> `-module-dir`, to `f18` (e.g. similarly to how `-fparse-only` and
> `-fsyntax-only` are handled)
Yes, in general when an option is given a different name in the new driver that
option should be added to f18. Not only for testing consistent behavior but
also because f18 gets more usage so any problems are more likely to turn up.
> **IMPLEMENTATION**
> If I understand correctly, the current implementation of `-fdefault-real-8`
> in `f18` is incomplete, see [[
> https://github.com/llvm/llvm-project/blob/b6db47d7e044730dc3c9b35dae6697eee0885dbf/flang/tools/f18/f18.cpp#L568-L569
> | here ]]:
> ```
> } else if (arg == "-r8" || arg == "-fdefault-real-8") {
> defaultKinds.set_defaultRealKind(8);
> ```
> I think that there should be `defaultKinds.set_defaultRealKind(16);` as well.
Do you mean `set_doublePrecisionKind(16)`? Yes, if that's how
`-fdefault-real-8` is supposed to behave, it should work that way in f18 too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96344/new/
https://reviews.llvm.org/D96344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments. Comment at: flang/test/Flang-Driver/fdefault.f90:10 +! RUN: not %flang-new -fsyntax-only -fdefault-double-8 %s 2>&1 | FileCheck %s --check-prefix=DOUBLE +! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new -fdefault-real-8 %s 2>&1 +! RUN: cat %t/dir-flang-new/m.mod | grep 'real_kind=8_4' awarzynski wrote: > tskeith wrote: > > I don't think you need to create a subdirectory. `%t` is a temp directory > > specific to this test. > > > > So I'm suggesting: > > ``` > > ! RUN: %flang -fsyntax-only -fdefault-real-8 %s > > ``` > From [[ https://llvm.org/docs/CommandGuide/lit.html#substitutions | LIT docs > ]]: > > ``` > %ttemporary file name unique to the test > %Tparent directory of %t (not unique, deprecated, do not use) > ``` > > So, IIUC, we do need to create a directory. We could skip `` > in the directory name, but it does no harm and can be really helpful when > parsing CI logs. > > I might be missing something though. @tskeith ? I thought lit created an empty directory for `%t` but apparently not. You just get whatever was left over from last run. So the safest thing to do is this for each compilation command: `! RUN: rm -rf %t && mkdir %t && %flang -module-dir %t ...` It's true that adding an extra subdirectory does no harm besides clutter, but what's the benefit? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments. Comment at: flang/test/Flang-Driver/fdefault.f90:4 + +! REQUIRES: new-flang-driver + Can't this work with the f18 driver too? That's the best way to ensure they are consistent. Comment at: flang/test/Flang-Driver/fdefault.f90:10 +! RUN: not %flang-new -fsyntax-only -fdefault-double-8 %s 2>&1 | FileCheck %s --check-prefix=DOUBLE +! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new -fdefault-real-8 %s 2>&1 +! RUN: cat %t/dir-flang-new/m.mod | grep 'real_kind=8_4' I don't think you need to create a subdirectory. `%t` is a temp directory specific to this test. So I'm suggesting: ``` ! RUN: %flang -fsyntax-only -fdefault-real-8 %s ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:254
+ if (args.hasArg(clang::driver::options::OPT_fdefault_real_8))
+res.defaultKinds().set_defaultRealKind(8);
+ if (args.hasArg(clang::driver::options::OPT_fdefault_integer_8)) {
awarzynski wrote:
> arnamoy10 wrote:
> > awarzynski wrote:
> > > From `gfortran` [[
> > > https://gcc.gnu.org/onlinedocs/gfortran/Fortran-Dialect-Options.html |
> > > documentation ]]:
> > >
> > > ```
> > > This option promotes the default width of DOUBLE PRECISION and double
> > > real constants like 1.d0 to 16 bytes if possible.
> > > ```
> > >
> > > So I believe that you are missing:
> > > ```
> > > res.defaultKinds().set_doublePrecisionKind(16);
> > > ```
> > I thought it is not necessary because `doublePrecisionKind` is
> > automatically initialized to double the size of `defaultRealKind_` in [[
> > https://github.com/llvm/llvm-project/blob/adfd3c7083f9808d145239153c10f72eece485d8/flang/include/flang/Common/default-kinds.h#L51-L58
> > | here ]]--> `int doublePrecisionKind_{2 * defaultRealKind_};`.
> >
> >
> Yes, but the default value for `defaultRealKind_` is 4 and here you are
> setting it to 8. So, correct me if I'm wrong, but when the driver is here,
> the following has happened:
> ```
> int defaultIntegerKind_{4};
> int defaultRealKind_{defaultIntegerKind_};
> int doublePrecisionKind_{2 * defaultRealKind_};
> ```
> So `dublePrecisionKind_` is 8 rather than 16.
You can test these are working correctly by compiling a module like this:
```
module m
implicit none
real :: x
double precision :: y
integer, parameter :: real_kind = kind(x)
integer, parameter :: double_kind = kind(y)
end
```
Then check for `double_kind=8_4` (or whatever) in the generated `.mod` file.
For `-flarge-sizes`:
```
real :: z(10)
integer, parameter :: size_kind = kind(ubound(z, 1))
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96344/new/
https://reviews.llvm.org/D96344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96875: [flang][driver] Add -fdebug-module-writer option
tskeith added inline comments. Comment at: flang/lib/Frontend/CompilerInvocation.cpp:234 + // -J/module-dir option + auto = res.moduleDir(); auto moduleDirList = In my opinion, using mutable references like this make the code hard to understand. Setters on CompilerInvocation would be clearer. Comment at: flang/test/Semantics/mod-file-rewriter.f90:9 +! RUN: %flang-new -fsyntax-only -fdebug-module-writer %p/Inputs/mod-file-unchanged.f90 2>&1 | FileCheck %s --check-prefix CHECK_UNCHANGED +! RUN: %flang-new -fsyntax-only -fdebug-module-writer %p/Inputs/mod-file-changed.f90 2>&1 | FileCheck %s --check-prefix CHECK_CHANGED Why do you need to duplicate these lines? Doesn't it work to use `%flang`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96875/new/ https://reviews.llvm.org/D96875 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes
tskeith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4231 +def flarge_sizes : Flag<["-"],"flarge-sizes">, Group, + HelpText<"Set the default KIND for INTEGER to 8.">; } That's not what -flarge-sizes does. Here is the description from flang/docs/Extensions.md: > The default `INTEGER` type is required by the standard to occupy > the same amount of storage as the default `REAL` type. Default > `REAL` is of course 32-bit IEEE-754 floating-point today. This legacy > rule imposes an artificially small constraint in some cases > where Fortran mandates that something have the default `INTEGER` > type: specifically, the results of references to the intrinsic functions > `SIZE`, `STORAGE_SIZE`,`LBOUND`, `UBOUND`, `SHAPE`, and the location > reductions > `FINDLOC`, `MAXLOC`, and `MINLOC` in the absence of an explicit > `KIND=` actual argument. We return `INTEGER(KIND=8)` by default in > these cases when the `-flarge-sizes` option is enabled. > `SIZEOF` and `C_SIZEOF` always return `INTEGER(KIND=8)`. Comment at: flang/lib/Frontend/CompilerInvocation.cpp:269 +} +res.defaultKinds().set_defaultRealKind(4); + } If `-fdefault-double-8` requires `-fdefault-real-8` then why is the default real kind set to 4? I don't understand the purpose of this option. The kind of DOUBLE PRECISION is already 8 so there is no need to change it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D96483: [flang][driver] Add options for unparsing
tskeith added inline comments.
Comment at: flang/tools/f18/f18.cpp:541
driver.debugNoSemantics = true;
-} else if (arg == "-funparse") {
+} else if (arg == "-funparse" || arg == "-fdebug_unparse") {
driver.dumpUnparse = true;
This should be "-fdebug-unparse" (with a hyphen). That may be why all those
tests are failing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96483/new/
https://reviews.llvm.org/D96483
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95448: [flang][driver] Add support for `-J/-module-dir`
tskeith added inline comments.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
/// {
+ Fortran::semantics::SemanticsContext () const { return
*semantics_; }
awarzynski wrote:
> tskeith wrote:
> > `semanticsContext` would be a better name for this function.
> We should follow Flang's [[
> https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md#naming
> | coding style ]] here:
> ```
> Accessor member functions are named with the non-public data member's name,
> less the trailing underscore.
> ```
> i.e. `semantics()` rather than `semanticsContext()`. If we were to diverge
> from that, then I suggest that we follow the style prevalent in LLVM/Clang,
> see e.g. [[
> https://github.com/llvm/llvm-project/blob/21bfd068b32ece1c6fbc912208e7cd1782a8c3fc/clang/include/clang/Frontend/CompilerInstance.h#L503-L506
> | getSema ]].
>
> @tskeith, I'm guessing that you wanted the member variable to be updated too:
> * semantics_ -> semanticsContext_
> Makes sense to me.
>
> @tskeith, I'm guessing that you wanted the member variable to be updated too:
> * semantics_ -> semanticsContext_
Right.
Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+ semantics_(new Fortran::semantics::SemanticsContext(*(new
Fortran::common::IntrinsicTypeDefaultKinds()),*(new
common::LanguageFeatureControl()),
+ *allCookedSources_)) {
awarzynski wrote:
> tskeith wrote:
> > Why is `semantics_` a `shared_ptr` rather than a simple data member of
> > type `SemanticsContext`? It's owned by only `CompilerInstance`, not shared.
> > The same question probably applies to the other fields too.
> @tskeith You raise two important points here:
>
> **Why shared_ptr if the resource is not shared?**
> From what I can see, at this point the `SemanticsContext` is not shared and
> we can safely use `unique_ptr` instead.
>
> **Why are semantics_ and other members of CompilerInstance pointers?**
> `CompilerInstance` doesn't really own much - it just encapsulates all
> classes/structs that are required for creating a _compiler instance_. It's
> kept lightweight and written in a way that makes it easy to _inject_ custom
> instances of these classes. This approach is expected to be helpful when
> creating new frontend actions (I expect that there will be a lot) and when
> compiling projects with many source files.
>
> @tskeith, I intend to document the design of the new driver soon and suggest
> that that's when we re-open the discussion on the design of
> `CompilerInstance`.
>
> IMO this change is consistent with the current design and I think that we
> should accept it as is.
>
> **Small suggestion**
> @arnamoy10 , I think that you can safely add `IntrinsicTypeDefaultKinds` and
> `LanguageFeatureControl` members too. We will need them shortly and this way
> this constructor becomes much cleaner. I'm fine either way!
>
>
> **Why are semantics_ and other members of CompilerInstance pointers?**
> `CompilerInstance` doesn't really own much - it just encapsulates all
> classes/structs that are required for creating a _compiler instance_.
As it stands it does own the instance of `SemanticsContext` etc. No one else
does.
> @tskeith, I intend to document the design of the new driver soon and suggest
> that that's when we re-open the discussion on the design of
> `CompilerInstance`.
OK
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95448/new/
https://reviews.llvm.org/D95448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95448: [flang][driver] Add support for `-J/-module-dir`
tskeith added inline comments.
Comment at: clang/include/clang/Driver/Options.td:1018
+ an USE statement. The default is the current
directory.}]>,Group;
+def module_dir : Separate<["-"], "module-dir">,
Flags<[FlangOption,FC1Option]>, MetaVarName<"">, Alias;
def dsym_dir : JoinedOrSeparate<["-"], "dsym-dir">,
It would be better to make `-module-dir` the main option and `-J` the alias.
`-J` is only there for gfortran compatibility, not because it is a good name
for the option.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:105
/// {
+ Fortran::semantics::SemanticsContext () const { return
*semantics_; }
`semanticsContext` would be a better name for this function.
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:67
+ // of options.
+ std::string moduleDirJ_ = ".";
+
`moduleDir_` would be a better name.
Comment at: flang/lib/Frontend/CompilerInstance.cpp:29
+ semantics_(new Fortran::semantics::SemanticsContext(*(new
Fortran::common::IntrinsicTypeDefaultKinds()),*(new
common::LanguageFeatureControl()),
+ *allCookedSources_)) {
Why is `semantics_` a `shared_ptr` rather than a simple data member of type
`SemanticsContext`? It's owned by only `CompilerInstance`, not shared.
The same question probably applies to the other fields too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95448/new/
https://reviews.llvm.org/D95448
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D95460: [flang][driver] Add forced form flags and -ffixed-line-length
tskeith added inline comments. Comment at: clang/include/clang/Driver/Options.td:4133 def fconvert_EQ : Joined<["-"], "fconvert=">, Group; -def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group; +def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Flags<[FlangOption, FC1Option]>, + Group, HelpText<"Use as character line width in fixed mode">, Why isn't this `-ffixed-line-length=` (i.e. with an equals sign, not a hyphen)? Isn't that how clang does options with values? It's fine to support the gfortran form as an alternative but I think the primary goal should be good consistent style for options. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95460/new/ https://reviews.llvm.org/D95460 ___ 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`
tskeith added inline comments. Comment at: flang/test/Flang-Driver/include-header.f90:59 +#endif +end `-I` also is supposed to affect INCLUDE lines so it would be good to have a test for that too. They are handled during preprocessing and so can be tested the same way. It also affects searching for .mod files but I think to test that requires semantics (i.e. to report an error if the module can't be found). It would be good to test that somewhere too. 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
[PATCH] D93401: [flang][driver] Add support for `-D`, `-U`
tskeith added inline comments.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:228
+ // Note: GCC drops anything following an end-of-line character.
+ llvm::StringRef::size_type End = MacroBody.find_first_of("\n\r");
+ MacroBody = MacroBody.substr(0, End);
This is a change in behavior from f18, right? If so, the commit message should
mention it and why it's changing.
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] D86089: [flang][driver]Add experimental flang driver and frontend with help screen
tskeith requested changes to this revision.
tskeith added inline comments.
This revision now requires changes to proceed.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+
Why is this called "Frontend" rather than "Driver"?
Comment at: flang/include/flang/Frontend/CompilerInstance.h:16
+
+namespace flang {
+
Nothing else is in namespace `flang`. `Fortran::frontend` would be more
consistent.
Comment at: flang/include/flang/Frontend/CompilerInstance.h:21
+ /// The options used in this compiler instance.
+ std::shared_ptr Invocation;
+
Data members, local variables, etc. should start with lower case.
Non-public data members should end with underscore.
Please follow the style in flang/documentation/C++style.md.
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31
+ Language Lang;
+ unsigned Fmt : 3;
+ unsigned Preprocessed : 1;
Why isn't the type `Format`?
Comment at: flang/include/flang/Frontend/FrontendOptions.h:32
+ unsigned Fmt : 3;
+ unsigned Preprocessed : 1;
+
Why isn't the type `bool`?
Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+ /// Show the -version text.
+ unsigned ShowVersion : 1;
+
Why aren't the types `bool`?
Comment at: flang/include/flang/FrontendTool/Utils.h:18
+
+#include
+
Is this needed?
Comment at: flang/lib/Frontend/CompilerInstance.cpp:39
+ } else
+Diags->setClient(new clang::TextDiagnosticPrinter(llvm::errs(), Opts));
+
The "then" and "else" statements should have braces around them.
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+ InputKind DashX(Language::Unknown);
+ return DashX;
+}
Why not `return InputKind{Language::Unknown};`?
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:91
+ InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags);
+ (void)DashX;
+
What is the purpose of `DashX` here?
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {
What is this for?
Comment at: flang/tools/flang-driver/driver.cpp:30
+extern int fc1_main(
+llvm::ArrayRef Argv, const char *Argv0, void *MainAddr);
+
Why isn't this declared in a header?
Comment at: flang/tools/flang-driver/fc1_main.cpp:32
+int fc1_main(
+llvm::ArrayRef Argv, const char *Argv0, void *MainAddr) {
+
MainAddr isn't used.
Comment at: flang/tools/flang-driver/fc1_main.cpp:54
+ // Execute the frontend actions.
+ { Success = ExecuteCompilerInvocation(Flang.get()); }
+
Why is this statement in a block? Is the result of CreateFromArgs supposed to
affect the return value of this function?
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
