This is an archive of the discontinued LLVM Phabricator instance.

[flang] Refine CR handling
ClosedPublic

Authored by klausler on Jul 14 2020, 12:47 PM.

Details

Summary

We need to retain carriage return characters in source files
that are not parts of multi-byte line endings; they are
significant in CHARACTER literal constants.

Diff Detail

Event Timeline

klausler created this revision.Jul 14 2020, 12:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
tskeith accepted this revision.Jul 14 2020, 12:51 PM
This revision is now accepted and ready to land.Jul 14 2020, 12:51 PM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Jul 15 2020, 1:58 AM

It sounds like this fixes a problem. Would it be possible to add a test?

It sounds like this fixes a problem. Would it be possible to add a test?

The FCVS test FM363 covers the issue. It would be helpful for the commit message to refer to the test.

The project is looking for volunteers to integrate and upstream this freely available Fortran test suite [1].

[1] https://www.itl.nist.gov/div897/ctg/fortran_form.htm

fhahn added a comment.Jul 15 2020, 6:45 AM

It sounds like this fixes a problem. Would it be possible to add a test?

The FCVS test FM363 covers the issue. It would be helpful for the commit message to refer to the test.

I see, but would it be hard to add such a unit test to the flang parser tests in tree? An external test suite is great, but most code should also be covered by unit-tests, unless it is not possible to do so.

Changing the topic a bit:

! In D83808#2152697, @fhahn wrote:
... An external test suite is great, ...

Are test suites in llvm-test-suite considered to be external?

fhahn added a comment.Jul 15 2020, 7:45 AM

Changing the topic a bit:

! In D83808#2152697, @fhahn wrote:
... An external test suite is great, ...

Are test suites in llvm-test-suite considered to be external?

Yes, I meant external to the llvm-project mono-repo. The tests in llvm-project are the first line of defense that developers use and IIUC they should exercise and test most of the code in llvm-projec It is also what most public buildbots run. For example, clang has an extensive test suite of the C/C++ parser in tree (https://github.com/llvm/llvm-project/tree/master/clang/test/Parser) , as well as benefiting from extensive external testing (bootstrap builds, llvm-test-suite and so on).

Given that the only public bot I am aware of (http://lab.llvm.org:8011/builders/flang-aarch64-ubuntu) only runs ninja check-all, it seems like good coverage of the flang parser (and other parts of flang) in tree would be quite important to catch regressions.

Of course there are cases that cannot be reasonably tested as tests in-tree, for example if the test would require very large input files. As I mentioned initially, I am not sure if this applies for a test for the issue this patch fixes.