[flang] Refine CR handling

[klausler]

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.

[fhahn]

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

[sscalpone]

In D83808#2152697, @fhahn wrote:

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]

In D83808#2153101, @sscalpone wrote:

In D83808#2152697, @fhahn wrote:

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.

[sscalpone]

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]

In D83808#2153235, @sscalpone wrote:

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.

flang/include/flang/Parser/char-buffer.h

public:
   // The return value is the byte offset of the new data,
   // i.e. the value of size() before the call.
   std::size_t Put(const char *data, std::size_t n);
   std::size_t Put(const std::string &);
   std::size_t Put(char x) { return Put(&x, 1); }
 
   std::string Marshal() const;
 
-  // Removes carriage returns ('\r') and ensures a final line feed ('\n').
-  std::string MarshalNormalized() const;
-
 private:
   struct Block {
     static constexpr std::size_t capacity{1 << 20};
     char data[capacity];
   };
 
   int LastBlockOffset() const { return bytes_ % Block::capacity; }
   std::forward_list<Block> blocks_;
   std::forward_list<Block>::iterator last_{blocks_.end()};
   std::size_t bytes_{0};
   bool lastBlockEmpty_{false};
 };
 } // namespace Fortran::parser
 #endif // FORTRAN_PARSER_CHAR_BUFFER_H_

flang/lib/Parser/char-buffer.cpp

for (std::size_t j{0}; j < chunk; ++j) {
       result += block.data[j];
     }
     bytes -= chunk;
   }
   result.shrink_to_fit();
   CHECK(result.size() == bytes_);
   return result;
 }
-
-std::string CharBuffer::MarshalNormalized() const {
-  std::string result;
-  std::size_t bytes{bytes_};
-  result.reserve(bytes + 1 /* for terminal line feed */);
-  char ch{'\0'};
-  for (const Block &block : blocks_) {
-    std::size_t chunk{std::min(bytes, Block::capacity)};
-    for (std::size_t j{0}; j < chunk; ++j) {
-      ch = block.data[j];
-      if (ch != '\r') {
-        result += ch;
-      }
-    }
-    bytes -= chunk;
-  }
-  if (ch != '\n') {
-    result += '\n';
-  }
-  result.shrink_to_fit();
-  return result;
-}
 } // namespace Fortran::parser

flang/lib/Parser/source.cpp

while (bytes > 0) {
     void *crvp{std::memchr(vp, '\r', bytes)};
     char *crcp{static_cast<char *>(crvp)};
     if (!crcp) {
       std::memmove(buffer + wrote, p, bytes);
       wrote += bytes;
       break;
     }
     std::size_t chunk = crcp - p;
+    auto advance{chunk + 1};
+    if (chunk + 1 >= bytes || crcp[1] == '\n') {
+      // CR followed by LF or EOF: omit
+    } else if ((chunk == 0 && p == buf.data()) || crcp[-1] == '\n') {
+      // CR preceded by LF or BOF: omit
+    } else {
+      // CR in line: retain
+      ++chunk;
+    }
     std::memmove(buffer + wrote, p, chunk);
     wrote += chunk;
-    p += chunk + 1;
-    bytes -= chunk + 1;
+    p += advance;
+    bytes -= advance;
   }
   return wrote;
 }
 
 bool SourceFile::Open(std::string path, llvm::raw_ostream &error) {
   Close();
   path_ = path;
   std::string errorPath{"'"s + path_ + "'"};