Thanks @awarzynski ! Yes, the namespace changes (and use of "static") were requested in a previous comment on this review :

The coding standard indicates that anonymous namespace should be made as small as possible and only include class declarations.
Functions should be made static and outside the namespace instead.

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

address latest review comments

I believe I have corrected all the build errors and addressed all the comments. There should now be appropriate working testing as well. Is this patch ready to go? Thanks!

clang-format

fix build error

Fix whitespace errors (hopefully)

Rebase on main

move definition of computeDerivedTypeSize to fix build error

Fixed build error - removed duplicate function definition

rebase on upstream main

Address review comments - clearer comments and reduced use of auto

Address review comments - add a test that triggers the TODO in AllocMemOp Conversion

rebase on main - add TODO back in
get successfull test of TODO in AllocMemOp converion of derived type with LEN params

Rebase on main

Remove TODO in favor of notifyMatchFailure

Fix a couple namespacing errors

Fix a couple namespacing errors

Rebase on main

Address review comments

• move CHECK blocks to after the mlir code in the test file
• fix style with respect to anonymous namespaces: only include class definitions in the namespace and make functions static and outside the namespace
• fix a few nits

Thanks so much for the review! I've been a little preoccupied of late, so this patch wasn't quite as polished as I would have like. Thanks for helping me get it in better shape!

Address review comments.

LGTM

Looks good to me.

LGTM, apart from a few minor typos in various comments.

In D109108#3027357, @sscalpone wrote:

Time to upgrade to C++20?
https://en.cppreference.com/w/cpp/types/endian

The idea seems solid to me, but I am also unsure how best to test this. Also, did you clang-format the code before uploading the patch?

@richard.barton.arm, I don't think I have any code that relies on __F18 being defined, so I'm alright with removing it and the related macros. You also make a very good point about following clang behavior. I wasn't aware of clang's use of lowercase macros and just assumed everyone uses uppercase macros based on my previous experiences with other codes. I think your proposal is likely the better way to go, first because matching style across LLVM projects is generally a good thing and second because your proposal does not require changes to classic Flang so we don't have to impose on those developers.

In D84334#2173076, @naromero77 wrote:

I am assuming that f18 will have a MAJOR version number matching the LLVM release? Does classic flang do that as well? Both compilers will probably co-exist for a while and it would be desirable to make them distinguishable to build systems as CMake.

I don't have a solution, but maybe we need to #define __CLASSIC_FLANG in classic flang. I am just throwing out ideas.

This looks good to me. I think these changes mean that the vector relocatables never actually gets populated, so it could be removed since the patch uses objlist and liblist exclusively, which are clearer names anyway.

I don't know what changed between my last patch and now, but I am unable to land this patch myself. I keep getting the following error:
EXCEPTION: (ConduitClientException) ERR-CONDUIT-CALL: Conduit API method "harbormaster.buildable.search" does not exist. at [<arcanist>/src/conduit/ConduitFuture.php:65]

Looks good to me. Please wait for approval from at least one of the other reviewers before merging.

Thanks for the patch! I had a couple questions and one small change request (see inline comments). The questions truly should be treated as questions rather than requests for changes, since I'm not 100% sure of the right answer to them.

@richard.barton.arm Is this patch acceptable now? Thanks!

Updates following a rebase on master (due to changes on D83687)

Extended the flang driver options to include gfortran equivalents to pgf90 specific options.

@richard.barton.arm I have made the requested adjustments in the other patch. Thanks for the feedback!

Correction for handling of OpenMP enabling flags

The second part of the split can be found here: https://reviews.llvm.org/D83687

Splitting this into two patches as requested by reviewers.

I'll admit, this is my first time trying to commit to LLVM directly and using Phabricator (rather than on a fork that uses pull requests), and I'm definitely still learning the workflow. I had some git issues when first creating the patch that meant my first commit was lost locally, so when I went to update the patch it only had the second commit which caused the original changes to disappear. I then created a new branch and re-did the changes and updated again, using only a single commit, so that the diff would now have everything in it.

In D83488#2142950, @clementval wrote:

I don't see the previous change made to set gfortran as default. Are they gone with your diff update?

Changed the default external compiler used by the flang temporary driver.

Extended the flang driver options to include gfortran equivalents to pgf90 specific options.