Option 1 for me, let's not delay.

Happy with the changes made since my comments.
Thanks Andrzej for the great documentation.

Great to see some docs added Andrzej - this should help us maintain the new Driver in future.

The change does NOT appear to be necessary for main of llvm-project, however I could reproduce it with fir-dev branch of f18-llvm-project. I was told that changes to this file should go through phabricator and percolate to fir-dev.

I agree with @tskeith that -Mstandard is not exactly orthogonal to -std, the former being about warning for non-standard extensions/deviations and the latter being about use of standard fortran, but from a different standard version. I would expect -Mstandard to apply to whatever value of -std you gave the compiler.

LGTM - I think this amounts to some dead code removal at the end of the day. Thanks!

Never got time to come back to this

I think the right long-term solution is going to be SUGGESTION 2, or a variant of that. We have implemented this feature downstream in Arm Compiler for Linux, i.e. a separate, normally longer and more detailed, Doc string for the CommandLineReference.rst file from the --help string. Such a mechanism could perhaps be extended to add Clang- and/or Flang- specific doc strings or additional doc strings. That's a big piece of work, but very worthy IMO.

Could we tweak the wording to clarify that it is Clang-specific?

The rationale for this revert looks good to me.

I'm happy to accept this revision based on @SouraVX 's code review and the fact that Andrzej has sent multiple RFCs covering the clang-side changes, including the Options flags (latest here http://lists.llvm.org/pipermail/cfe-dev/2020-October/067079.html). I think this code is good enough to commit.

Looks like I committed this without the link text as 69230e7

Sorry @sameeranjoshi - you are right, the changes look good.

@sameeranjoshi I see this merged but I don't see any changes to the sidebar in my local build.

Gone for the merge.

Hi @hans thanks for giving it a try. I think it is fair enough that you don't try to fix it yourself.

LGTM - thanks for this.

NP. We need to hurry up and get that buildbot into stable. We are upgrading the machine it runs on so it doesn't take 4 hours to build+make check! Hoping to have it up next week.

Nice one - a good addition to the docs, IMO.

@sameeranjoshi you happy to "accept this revision" :-) ?

I don't see a difference in the two for style. Both seem to have previous | next | index links in the top (and bottom) bars, as well as the indexsidebar as you point out. The indexsidebar seems to be a standard default from sphinx. LLVM uses a special one in llvm/docs/_templates/indexsidebar.html which does not have the previous/next topic links and instead has the permalinks to other articles.

I was talking about having the Table Of Contents which is in right pane to be in the center of page the way LLVM has.
Also when D87226 would be applied, it would remove the TOC and will add other contents there.

I've posted what I think is the fix over on https://reviews.llvm.org/D87505

This commit missed adding a dependency from libFortranEvaluate and libFortranSemantics. Adding it back in introduces a circular dependency and so breaks shared library builds.

LGTM

Rebasing over the top of the license header restoration.

The TOC tree is displayed in the right indexsidebar, if it could be displayed at the top (see http://llvm.org/docs/CMake.html for reference) of individual pages that would be better and it would help reader initially to navigate to the internal section in the respective page.
Further I went on to apply D87226(contains a fixed side bar index links ) and the TOC disappears probably due to above mentioned issue.

This LGTM. I think all the previous comments from other reviewers and me have been dealt with so I am happy to accept this revision based on the reviews so far.

New version that uses recommonmark's AutoStructify plugin to re-write
the index page in markdown with toctrees in embedded rst blocks.

In D87242#2259691, @sameeranjoshi wrote:

I have a GettingInvolved.md can you include it as well if it makes sense to add it with this patch and the original authors agree.
Most of the content comes from mailing list meeting minutes from Gary(Thanks @gak).

GettingInvolved.md3 KBDownload

Another random thought that just came to me: what does the new driver do when you invoke it with no input files or options? I could imagine a few sensible outcomes (error: no input (clang and gcc/gfortran behaviour), print version and exit, print help and exit, etc.) and I don't have a strong preference over which we chose here but I think there should be a test for it in test/Driver

Requesting changes mostly because of the exit status issue on the Driver tests.

I have tested this works well on an out of tree build (as it broke it before) so have no reservations about pushing the change again on @coti 's behalf.

This is working well for me in an in-tree build.

@coti the reason for the build failure is because you have removed both of the duplicate definitions of include in flang/tools/f18/CMakeLists.txt. This causes CMake not to be able to find the right files when it creates the builtin module files. One of these needs to be restored in your patch for the build to work as before.

A few specific comments to address here as well as the pre-commit linting ones.

Steve's patch works for me (cmake 3.17). @coti , your error message looks like what would happen if you replaced both definitions of {{include}} with the {{target_include_directories}} call. {{include}} is set up to the location of pre-built modules and is used in the compilation of said modules and then pre-processed into the flang.sh script. You still need that as well as setting {{target_include_directories}} to tell CMake where f18_version.h is when building f18.cpp.

@coti - I've had another look and I think your new test is stumbling across an existing bug. If you play around with the existing __F18* macros, you can see that they are not being applied correctly in the code:

I still see a few mentions of the old documentation dir location in the codebase:

I have reverted the patch

These changes build for me and I get a .html version of the flang release notes in the docs directory in the build. It is a shame there is not more content there and I suppose we'll need to convert the docs in /documentation over to .rst format to get them included in this set.

Please can you send me your email address offline for the commit log?

I'm afraid arcanist does not apply the patch well. It does not like the new file creation. I'm not sure how you are creating this diff, but are you able to re-upload a diff created with diff -Naur to handle the new files correctly?

I'm afraid that still doesn't work. My master is on 7d0f691.

Regarding the release note contents. My original review proposing these did not get any strong objections so I propose that once we are all happy with the code that this initial version of the release notes be committed alongside. I will make a follow up review with any further edits to it before release.

It needs rebasing - please can you do this then I will commit it on your behalf?

Do you have commit access @coti ?

Abandoning the changes in favour of https://reviews.llvm.org/D85470

Super - thanks @sameeranjoshi for taking this forward. I'll close this patch and we can do the rest of the work on D85470

Thanks for adding the tests. IMO, the regex approach in the test is just fine.

The preprocessing tests are being executed now so that would be a good place to add a test.

Looking good. Please can you add some tests?

• For --version, there already is one for the version screen in flang/test/Driver that could be updated to capture the version number also and run multiple times to test the synonyms.
  • Testing {{\-v}} when it means verbose is hard so suggest not doing that. We need {{-###}} really to test the driver more, which is outside the scope of your change.
• For the macros, I think the flang/test/Preprocessing" directory would be the best place but that directory is full of non-tests (which we need to delete really) so we can't add a real test there at the moment. Perhaps add a new test to flang/test/Driver for now that runs flang with -E and checks the substitutions work.

Fix clang-format error.

I don't think it is safe to assume that LLVM Flang will always be the same as classic Flang for all of the same behaviors that CMake cares about. I think we should be considering LLVM Flang as a new compiler rather than as a different version of classic Flang. I think that means we need to make a change in CMake to add support for LLVM Flang as a new compiler.

I wanted to push this as a patch series.

I wanted to push this as a patch series.

Although LLVM flang is the spiritual successor to classic flang, I don't think we are specifically aiming for any sort of compatibility between the two compilers, for example on option names, feature support, language support, processor defined behavior, 'bug compatibility', etc. so in this sense the two are completely different compilers. I think it unlikely that a user of classic flang that has conditionalized some code on __FLANG due to some quirk of classic flang or some bug in classic flang that they are working around will necessarily need or want to run the same code for LLVM flang for the same reason. I think this would steer us strongly away from supporting __FLANG in LLVM flang to indicate that it and classic flang are connected in this way.