Add unit tests for Fortran's type finalization feature

[rouson]

No description provided.

[rovka]

Thanks for working on this! I have a couple of comments inline, and also a more general question: Is there any magic you could use to disable some or all of these tests if the compiler doesn't support this feature?

[rovka]

List-directed output leaves room for all sorts of delimiters between values (one or more blanks, comma, semicolon), so it makes it more difficult to compare the output with a reference. I would recommend using a well-defined format, to force all compilers to produce the exact same output.

[rovka]

This adds an executable, but doesn't actually run the test. You should add a reference output and some boilerplate similar to what's been done for SNAP. Note that that implementation is a bit messy at the moment - if you have some cycles to spend on this, you could try to extract some CMake helpers to use both here and in SNAP, similar to what we have for C++ tests.

[kiranchandramohan]

@rovka Are Pull Requests accepted in this repository?

[rovka]

@kiranchandramohan I'm seeing a "Merge pull request" button, so... maybe? In any case, that's a good point. I don't mind reviewing here, but we should probably add a link to the PR in the commit message (similar to how we have the "Differential Revision" tag for Phabricator reviews), so people can easily find the discussion.

[clementval]

@rouson There is a patch up for review for the derived-type finalization in flang (https://reviews.llvm.org/D142707). It currently passes 9/10 tests. BLOCK construct is not yet implemented.
The specification expression test needs to be compiled and run separately since the compile_command, execute_command_line and compiler_version are still TODOs.

[rouson]

@clementval that's great progress! Thanks for the update.

[Meinersbur]

In principle, the same policies as for https://github.com/llvm/llvm-project applies: It has a bot that automatically closes all pull requests (see e.g. llvm/llvm-project#60154), but it seems that they are not configured the same way.

@kiranchandramohan I'm seeing a "Merge pull request" button, so... maybe?

That not good, we still don't want merge commits, but a linear history. Rebase should be the only option.

[zbeekman]

Greetings @rovka, @kiranchandramohan, @clementval and @Meinersbur. @rouson has tasked me with getting this submission to llvm-test-suite over the finish line. I have relatively extensive CMake and git experience, so, from a technical perspective I'm relatively comfortable with the feedback so far, however this is my first endeavor submitting a patch to LLVM.

I have read https://llvm.org/docs/Contributing.html and https://llvm.org/docs/Phabricator.html and have created a phabricator account (https://reviews.llvm.org/p/zbeekman/) but I'm still wading through some of the details and trying to figure out how to set up git-clang-format the fastest/most straightforward way on macOS and trying to decipher how to submit the patch with phabricator/arcanist. Any additional tips/tricks/concise advice is certainly welcome.

[clementval]

I think contribution to the test suite can go through github PR directly. No need for a phab review.

Last time I check there was some tests requiring "not yet implemented" features. Do you plan to implement those or disable the tests for the moment?

[zbeekman]

I think contribution to the test suite can go through github PR directly. No need for a phab review.

OK, great! That's a bit of a relief. I should still probably figure out phabricator and the normal review process but I'm glad to hear I can iterate on GitHub.

Last time I check there was some tests requiring "not yet implemented" features. Do you plan to implement those or disable the tests for the moment?

I don't believe I have the expertise required to implement the tests that fail because they rely on "not yet implemented" features. If there is a canonical way to mark a test as being relevant to a not-yet-implemented feature please let me know. Otherwise I could set the WILL_FAIL property on the test and add a comment to the CMake files so that once implemented the property can be removed.

From your previous comment:

@rouson There is a patch up for review for the derived-type finalization in flang (https://reviews.llvm.org/D142707). It currently passes 9/10 tests. BLOCK construct is not yet implemented.

I can mark the test for BLOCK construct as WILL_FAIL in CMake, or follow whatever the recommended best practice is for tests of heretofore unimplemented features.

The specification expression test needs to be compiled and run separately since the compile_command, execute_command_line and compiler_version are still TODOs.

The preferred approach is for all the relevant testing code to be compiled and run via CMake, and trying to match the style of existing flang/llvm infrastructure, correct?

[clementval]

I can mark the test for BLOCK construct as WILL_FAIL in CMake, or follow whatever the recommended best practice is for tests of heretofore unimplemented features.

BLOCK should be fine now. The test that compile and execute the second file should give you build failure with not yet implemented error.

[joker-eph]

I think contribution to the test suite can go through github PR directly. No need for a phab review.

I don't see any pull request ever landed in this repo (all commits seem reviewed on Phab instead), did anything change recently?

[clementval]

I think contribution to the test suite can go through github PR directly. No need for a phab review.

I don't see any pull request ever landed in this repo (all commits seem reviewed on Phab instead), did anything change recently?

Right. I was puzzled by the fact we can open PR here. But @joker-eph is right, phab is the way also here.

[zbeekman]

@clementval @rovka:

Ok, it seems like I need to go through phab.. I'll have to figure out how to finish getting that setup and how to initiate the process. In the meantime, any feedback that you are able to provide here would be most welcome (hopefully to reduce cycles spent going back and forth in phabricator).

I have:

• Broken the tests into individual files.
• Replaced list directed IO with well formatted (I hope!) repeatable edit descriptors (I'm just using "(A)".
• Created a .reference_output file for each test
• Integrated the tests with CMake using llvm_singlesource()

To attempt to keep the tests in a single source file from CMake's perspective and to strike a balance between complexity and avoiding duplicating code, I used old school include statements (include "object_type_m.f90") at the top of most test source files to pull in most of a relatively small module that defines a few derived types, procedures, module variables etc. Stylistically speaking I don't love this approach, however, there's no other immediately obvious good way to do this without a lot of code duplication.

@rouson: All the tests pass with flang-new as of at least today: d585a8afdf2f70159759dccb11d775cdf432aba4

...
size..gnu.hash: 28
size..gnu.version: 124
size..gnu.version_r: 80
size..got: 64
size..got.plt: 440
size..init: 27
size..init_array: 16
size..interp: 28
size..note.ABI-tag: 32
size..plt: 848
size..rela.dyn: 23904
size..rela.plt: 1248
size..rodata: 28640
size..text: 577202
**********

Testing Time: 0.06s
  Passed: 10

[clementval]

I guess you need to update the results here.

[clementval]

Besides updating the README with your latest result I think you can go ahead and submit a phab review.

[zbeekman]

Shoot, yes I need to update the README here/on my phab request.

…

On Fri, Apr 7, 2023 at 3:11 PM Valentin Clement (バレンタイン クレメン) < ***@***.***> wrote: Besides updating the README with your latest result I think you can go ahead and submit a phab review. — Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEIPGJ2GV5G526O2AQBITXABRE5ANCNFSM6AAAAAAQN7HCPE> . You are receiving this because you commented.Message ID: ***@***.***>

[tarunprabhu]

This has been merged in 65b897.