This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Parse Verilog if statements
ClosedPublic

Authored by sstwcw on Apr 9 2022, 3:47 AM.

Details

Summary

This patch mainly handles treating begin as block openers.

While and for statements will be handled in another patch.

Diff Detail

Event Timeline

sstwcw created this revision.Apr 9 2022, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2022, 3:47 AM
Herald added a subscriber: mgorny. · View Herald Transcript
sstwcw requested review of this revision.Apr 9 2022, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2022, 3:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sstwcw added inline comments.Apr 9 2022, 3:55 AM
clang/lib/Format/FormatToken.h
1122

Does anyone know why this part gets aligned unlike the two lists above?

clang/lib/Format/FormatToken.h
373

Can't we do that with a type?

I'm not very happy about the alias, because you can still call Tok.getKind().

1122

Have you reformatted the other lines with the same config and revision?
If yes, my guess would be the missing comment.

1499

So you have a blacklist what is not a keyword? Seems a bit non future proof, new C++ keywords would have to be added here.

1561

For consistency reasons add the comment like above.

sstwcw marked 3 inline comments as done.Apr 11 2022, 3:20 PM
sstwcw added inline comments.
clang/lib/Format/FormatToken.h
373

The main problem I seek to solve with the alias thing is with tok::hash. In Verilog they use a backtick instead of a hash. At first I modified all places in the code to recognize the backtick. MyDeveloperDay said "year really not nice.. its like we can never use tok::hash again!" Using a type also requires modifying all instances of tok::hash if I get you right. How do I please everyone?

1122

It was the missing comment.

1499

This is a whitelist of what is a keyword.

sstwcw updated this revision to Diff 422049.Apr 11 2022, 3:22 PM
sstwcw marked 2 inline comments as done.

add comment

clang/lib/Format/FormatToken.h
373

Then you must hide Tok in the private part, so that no one can ever access Tok.getKind() accidentally.

1499

My bad. Go on.

sstwcw updated this revision to Diff 422496.Apr 13 2022, 6:06 AM
sstwcw edited the summary of this revision. (Show Details)

abandon alias

sstwcw marked an inline comment as done.Apr 13 2022, 6:12 AM
sstwcw added inline comments.
clang/lib/Format/FormatToken.h
373

In the new version the type of Tok itself gets changed instead. So an alias is not needed.

clang/lib/Format/FormatToken.h
1549

Shouldn't we add TT_MacroBlockBegin?

As first passes for adding a new language I think this looks pretty good.

`if (Keywords.isBlockBegin(*FormatTok, Style)) {`

I'm not a massive fan of handling l_brace and begin and end as the same thing, I might be tempted to handle them separately, as then its clear what is happening, but I don't think I'd block it for that.

sstwcw updated this revision to Diff 435771.Jun 9 2022, 7:34 PM
sstwcw marked 2 inline comments as done.

add MacroBlockBegin

sstwcw marked an inline comment as done.Jun 9 2022, 7:34 PM
sstwcw updated this revision to Diff 435773.Jun 9 2022, 7:38 PM

use isVerilog

sstwcw updated this revision to Diff 435774.Jun 9 2022, 7:53 PM
sstwcw updated this revision to Diff 435777.Jun 9 2022, 8:11 PM
  • add brace
This revision is now accepted and ready to land.Jun 13 2022, 3:42 AM
This revision was landed with ongoing or failed builds.Jun 25 2022, 7:21 PM
This revision was automatically updated to reflect the committed changes.

clang/docs/ClangFormatStyleOptions.rst was out of sync because dump_format_style.py had not been run. Fixed in cc55d97.

owenpan added inline comments.Sep 1 2022, 11:28 PM
clang/lib/Format/UnwrappedLineParser.cpp
2593

This is likely the culprit of https://github.com/llvm/llvm-project/issues/57509. @sstwcw can you have a look?

owenpan added inline comments.Sep 2 2022, 9:00 PM
clang/lib/Format/FormatToken.h
1550

Due of what isBlockBegin() is replacing in this patch, TT_MacroBlockBegin should be added in a separate patch (if it's correct to add it) along with corresponding unit tests.

owenpan added inline comments.Sep 2 2022, 9:24 PM
clang/lib/Format/UnwrappedLineParser.cpp
2593

`if (Keywords.isBlockBegin(*FormatTok, Style)) {`

I'm not a massive fan of handling l_brace and begin and end as the same thing, I might be tempted to handle them separately, as then its clear what is happening,

+1

owenpan added inline comments.Dec 4 2022, 3:35 PM
clang/lib/Format/FormatToken.h
1549

Adding TT_MacroBlockBegin here caused an assertion failure. See also D123450#3768367.