This is an archive of the discontinued LLVM Phabricator instance.

[mlir][docgen] Handle Windows line endings in doc generation.
ClosedPublic

Authored by scotttodd on Jun 22 2023, 3:29 PM.

Details

Summary

The printReindented function searches for Unix style line endings (\n), but strings may have Windows style line endings (\r\n). Prior to this change, generated document sections could have extra indentation, which some markdown renderers interpret as code blocks rather than paragraphs.

Diff Detail

Event Timeline

scotttodd created this revision.Jun 22 2023, 3:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2023, 3:29 PM
scotttodd published this revision for review.Jun 22 2023, 3:31 PM
scotttodd added a reviewer: jpienaar.
jpienaar added inline comments.Jun 22 2023, 10:29 PM
mlir/include/mlir/Support/IndentedOstream.h
117

Could we use rtrim('\\r') on the StringRef of the line?

Switch from copy/remove/erase to rtrim.

Fix indent in second loop.

I'm thoroughly confused by arc and Phabricator, but I updated the change a few times...

mlir/include/mlir/Support/IndentedOstream.h
117

Yep! Here's the code flow on the test case now (note that the output now has "\r\n", but the indentation is correct):

// Unix
"\n\n\n         First line\n                 second line"

// split 1:
""
"\n\n         First line\n                 second line"

// split 2:
""
"\n         First line\n                 second line"

// -----------------------------

// Windows
"\r\n\r\n\r\n         First line\r\n                 second line"

// split 1:
"\r"  // <-- trim this so it's empty string
"\r\n\r\n         First line\r\n                 second line"

Squash rebase changes... maybe a single diff in Phabricator now?

scotttodd marked an inline comment as done.Jun 23 2023, 3:24 PM
jpienaar accepted this revision.Jun 23 2023, 4:37 PM
This revision is now accepted and ready to land.Jun 23 2023, 4:37 PM

I don't think I have write access to land this myself. If I did, I'd probably wait until Monday... but I wouldn't object to someone merging earlier than that.

Okay, we're back in typical business hours now. Could you / someone merge this for me? I can also see about getting commit access myself.