This is an archive of the discontinued LLVM Phabricator instance.

Make test_symbols.py compare files line-by-line
ClosedPublic

Authored by ijan1 on Aug 12 2021, 1:54 AM.

Details

Summary

We currently feed full files to Python's unified_diff.
It's not quite what we want though -
line-by-line comparison makes more sense
(we want to be able to identify missing/unnecessary lines)
and is also easier to parse for humans.
This patch makes sure that we compare one line at a time.

This change pretties up the output formatting in the script.
Output before:

!DEF:/m/s/xINTENT(IN)(Implicit)ObjectEntityREAL(4)
!DEF:/m/s/yINTENT(INOUT)(Implicit)ObjectEntityREAL(4)
-!-D-E-F-:-f-o-o-b-a-r-
puresubroutines(x,y)bind(c)
!REF:/m/s/x
intent(in)::x

Proposed output after:

!DEF:/m/s/xINTENT(IN)(Implicit)ObjectEntityREAL(4)
!DEF:/m/s/yINTENT(INOUT)(Implicit)ObjectEntityREAL(4)
-!DEF:foobar
puresubroutines(x,y)bind(c)
!REF:/m/s/x
intent(in)::x

Diff Detail

Event Timeline

ijan1 requested review of this revision.Aug 12 2021, 1:54 AM
ijan1 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2021, 1:54 AM
ijan1 edited the summary of this revision. (Show Details)Aug 12 2021, 1:55 AM
ijan1 edited the summary of this revision. (Show Details)

Makes a lot of sense, but I would rephrase the summary a bit. Basically, you are making sure here that you are comparing the files line by line rather than the whole thing at a time. By making sure that the comparison is done line-by-line, you also make the output much nicer. Perhaps:

Make test_symbols.py compare files line-by-line

We currently feed full files to Python's unified_diff. It's not quite what we want though - line-by-line comparison makes more sense (we want to be able to identify missing/unnecessary lines) and is also easier to parse for humans. This patch makes sure that we compare one line at a time.

This change pretties up the output formatting in the following way:
Output before:

--- after
+++ before
@@ -6,13 +6,13 @@
!DEF:/m/s/xINTENT(IN)(Implicit)ObjectEntityREAL(4)
!DEF:/m/s/yINTENT(INOUT)(Implicit)ObjectEntityREAL(4)
-!-D-E-F-:-f-o-o-b-a-r-
puresubroutines(x,y)bind(c)
!REF:/m/s/x
intent(in)::x

Proposed output after:

--- after
+++ before
@@ -6,13 +6,13 @@
!DEF:/m/s/xINTENT(IN)(Implicit)ObjectEntityREAL(4)
!DEF:/m/s/yINTENT(INOUT)(Implicit)ObjectEntityREAL(4)
-!DEF:foobar
puresubroutines(x,y)bind(c)
!REF:/m/s/x
intent(in)::x

What do you think? (btw, line numbers ^^^ need tweaking)

Meinersbur added inline comments.Aug 12 2021, 11:27 AM
flang/test/Semantics/test_symbols.py
46–48
ijan1 updated this revision to Diff 366836.Aug 17 2021, 1:53 AM

Updated to use join with unified_diff.

ijan1 retitled this revision from [Flang] test_symbols.py formatting fix to Make test_symbols.py compare files line-by-line.Aug 17 2021, 2:39 AM
ijan1 edited the summary of this revision. (Show Details)
ijan1 marked an inline comment as done.

Makes a lot of sense, but I would rephrase the summary a bit. Basically, you are making sure here that you are comparing the files line by line rather than the whole thing at a time. By making sure that the comparison is done line-by-line, you also make the output much nicer. Perhaps:

Thanks for the suggested summary! I've updated accordingly.

awarzynski accepted this revision.Aug 17 2021, 5:13 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 17 2021, 5:13 AM
ijan1 added a comment.Aug 19 2021, 1:15 AM

@Meinersbur is this ready to be merged?

This revision was automatically updated to reflect the committed changes.