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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2241
 
+**BreakBetweenInstancePorts** (``Boolean``) :versionbadge:`clang-format 15`
+  For Verilog, put each port on its own line in module instantiations.
----------------
I don't feel "InstancePorts" is clear here I assume this is a Verilog thing, if we are going to have Verilog specific options I think they should be prefixed with the name

`VerilogBreakBetweenInstancePorts`


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3207
+  * ``LK_Verilog`` (in configuration: ``Verilog``)
+    Should be used for Verilog / SystemVerilog.
+
----------------
Can we have a hyperlink? to what we are following


================
Comment at: clang/include/clang/Format/Format.h:1687
+  /// \version 15
+  bool BreakBetweenInstancePorts;
+
----------------
I'd like to see this be a general option that could be used for all languages


================
Comment at: clang/include/clang/Format/Format.h:4230
+/// Returns the description for assume-filename.
+StringRef getAssumeFilenameHelp();
+
----------------
This is part of in my view a different change which could be separate, I've seen this logged before


================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:1043
+      (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths &&
+       State.Line->First->is(tok::kw_enum)))
     return (Style.IndentWidth * State.Line->First->IndentLevel) +
----------------
Could you point me to the "unit test" that covers this case


================
Comment at: clang/lib/Format/Format.cpp:697
                    Style.BreakBeforeBinaryOperators);
+    IO.mapOptional("BreakBeforeBraces", Style.BreakBeforeBraces);
     IO.mapOptional("BreakBeforeConceptDeclarations",
----------------
given the size of your patch I'd break these kinds of unrelated changes out we can do an NFC patch

If you don't have commit access you need to get that first. I doubt someone else is going to land this on your behalf, you need to own it and you need to be committed to supporting this longer term (are you?)


================
Comment at: clang/lib/Format/Format.cpp:1210
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
   LLVMStyle.BreakBeforeConceptDeclarations = FormatStyle::BBCDS_Always;
----------------
Ditto above


================
Comment at: clang/lib/Format/Format.cpp:1213
   LLVMStyle.BreakBeforeTernaryOperators = true;
-  LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
+  LLVMStyle.BreakBetweenInstancePorts = true;
   LLVMStyle.BraceWrapping = {/*AfterCaseLabel=*/false,
----------------
I'm not going to lie I don't like the name "InstancePorts" it doesn't tell me what its doing.. is this a known technical term?


================
Comment at: clang/lib/Format/Format.cpp:1266
+  LLVMStyle.IndentCaseLabels =
+      Language == FormatStyle::LK_Verilog ? true : false;
   LLVMStyle.IndentCaseBlocks = false;
----------------
can we have isVerilog() I hate all these `==` `!=`


================
Comment at: clang/lib/Format/Format.cpp:3408
 
+static const ArrayRef<
+    std::pair<FormatStyle::LanguageKind, std::vector<const char *>>>
----------------
This is in my view an unrelated change break it out


================
Comment at: clang/lib/Format/FormatToken.h:62
   TYPE(FunctionTypeLParen)                                                     \
+  TYPE(GotoLabelColon)                                                         \
   TYPE(IfMacro)                                                                \
----------------
I feel this is a separate patch


================
Comment at: clang/lib/Format/FormatToken.h:80
   TYPE(JsAndAndEqual)                                                          \
-  TYPE(LambdaArrow)                                                            \
+  TYPE(LambdaArrow) /* Java lambda arrow, abused to mark the Verilog           \
+                       implication operator */                                 \
----------------
comment?


================
Comment at: clang/lib/Format/FormatToken.h:115
   TYPE(SelectorName)                                                           \
-  TYPE(StartOfName)                                                            \
+  TYPE(StartOfName) /* the first name in a list of declarations, like `a` in   \
+                       `int a, b;` */                                          \
----------------
is this comment always correct?


================
Comment at: clang/lib/Format/FormatToken.h:924
   /* Context-sensitive */                                                      \
-  KEYWORD(final, 0)                                                            \
+  KEYWORD(final, ATTR_VERILOG_KEYWORD)                                         \
   KEYWORD(override, ATTR_JS_KEYWORD | ATTR_CSHARP_KEYWORD)                     \
----------------
final is a CSHARP and C++ keyword too


================
Comment at: clang/lib/Format/FormatToken.h:1029
+   * has have different precedence. So they are defined here. */               \
+  KEYWORD(and, ATTR_VERILOG_KEYWORD)                                           \
+  KEYWORD(assign, ATTR_VERILOG_KEYWORD)                                        \
----------------
and is a C++ keyword right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121758