[PATCH] D121758: [clang-format] Add support for formatting Verilog code

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

You need to rebase the patch, it doesn't apply.



================
Comment at: clang/lib/Format/Format.cpp:3451
+    for (auto Suffix : Lang.second)
+      if (FileName.endswith(Suffix))
+        return Lang.first;
----------------
krasimir wrote:
> I kinda prefer the old style of handling this I find it simpler to read. I'd be OK if getAssumeFilenameHelp didn't print the mapping. Also the suffix array has a bit implicit semantics going on that it will be the first suffix matching, which isn't immediately clear in isolation.
> Also it looks like this changes behavior: in the old case, some of the matches were case-insensitive.
I agree with @krasimir here, 

Actually there isn't good alignment with the clang-format frontend and the library or any of the python or vim plugin scripts either over what extensions are/are not supported..

Very much a separate patch to cover everything in my view.

Also I've seen requests were we should not "guess a language" that isn't in the .clang-format style and that feels kind of related too..

i.e. those that use ObjC only and only have Language: ObjectiveC in their clang-format file probably want extensions mapped to ObjC


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:534
         continue;
-      parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
-                 /*MunchSemi=*/true, /*UnindentWhitesmithBraces=*/false,
-                 CanContainBracedList,
+      parseBlock(/*Flags=*/CanContainBracedList * BLOCK_CAN_CONTAIN_BRACED_LIST,
+                 /*AddLevels=*/1u,
----------------
sstwcw wrote:
> One of the people in charge said multiplying with a boolean might trigger warnings.  Here I compiled with gcc.  This version doesn't trigger warnings.  The other way to do it, `CanContainBracedList ? BLOCK_CAN_CONTAIN_BRACED_LIST : 0`, triggers a warning that I shouldn't mix enum and integer.
this is a hard no from me.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:559
+      {
+        ScopedTokenPosition AutoPosition(Tokens);
+        do {
----------------
could be separate small patch


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:580
+        // JavaScript: A 'case: string' style field declaration.
+        ParseDefault();
         break;
----------------
I think we are losing clarity here, ParseDefault does more than just parseStructuralElement its inverting the HasOpeningBrace and I just can't tell if the implications here would have knock on effects.

If ParseDefault was suitable here why did the author of ParseDefault not  cover that?



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4499-4500
+    while (!Line->InPPDirective && Keywords.isPPHash(*FormatTok, Style) &&
+           (Style.Language != FormatStyle::LK_Verilog ||
+            Keywords.isVerilogPPDirective(*Tokens->peekNextToken())) &&
            FirstNonCommentOnLine) {
----------------
doesn't the first line of `isVerilogPPDirective` say if  Language!=Verilog return false, if so is the Language check needed here?


================
Comment at: clang/lib/Format/UnwrappedLineParser.h:110
+    BLOCK_VERILOG_HIER = 0x10
+  };
+  IfStmtKind parseBlock(unsigned Flags = 0u, unsigned AddLevels = 1u,
----------------
Oh! please no... I can't say how much  of my life has been ruined by flags and the various `|  || & && ~&` bugs,  I'm sorry I'd rather have a structure and it be clear


================
Comment at: clang/unittests/Format/FormatTestUtils.h:22
 
-inline std::string messUp(llvm::StringRef Code) {
+inline std::string messUp(llvm::StringRef Code, bool HandleHash = true) {
   std::string MessedUp(Code.str());
----------------
Can you add a comment as to why I would need to `HandleHash=false`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121758/new/

https://reviews.llvm.org/D121758