[PATCH] D117416: [clang-format] Handle C variables with name that matches c++ access specifier

[MyDeveloperDay via Phabricator via llvm-commits]

MyDeveloperDay added a comment.

I actually thought you were on the right lines with your change to parseAccessSpecifier() surely if you see "private:"  "public:" or "protected:" but not `public::` that's when you call parseAccessSpecifier()

I think mostly I believe clang-format is a 96% done deal in terms of code, a lot of what we do here in my view is about closing those final "corner cases", sometimes our fixes are quite specific,

Something in the back of my head says this is a `sledge hammer to crack a nut` but that is just my view.. I'm just a little uncomfortable here with this change although it may open doors for other C specific changes, I'm not convinced this solves the problem I just think it moves it a little and maybe adds to complexity for what is likely a very small number of cases.



================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3238
                spaceRequiredBeforeParens(Right);
-      if (Left.isOneOf(tok::kw_new, tok::kw_delete))
+      if (Style.Language != FormatStyle::LK_C &&
+          Left.isOneOf(tok::kw_new, tok::kw_delete))
----------------
I wonder how many people using C do this?

```
#define new(x) malloc(x)
#define delete(x) free(x)
```

Some at least

https://github.com/jbf/tonsai-scheme/blob/83d2a022f60fc17f9913c017fbde29e19cdf5b2b/src/memory.h


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:100
   int getIndentOffset(const FormatToken &RootToken) {
-    if (Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
+    if (Style.Language == FormatStyle::LK_C ||
+        Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
----------------
isC() these large OR/AND conditions become unreadable


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1221
   case tok::kw_private:
-    if (Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
+    if (Style.Language == FormatStyle::LK_C ||
+        Style.Language == FormatStyle::LK_Java || Style.isJavaScript() ||
----------------
isC()


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1606
 
-      if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
+      if ((Style.isCpp() || Style.isC()) && FormatTok->is(TT_StatementMacro)) {
         parseStatementMacro();
----------------
I feel everywhere you put isCpp you'll end up doing isCpp() || is C()

This goes back to the arguments for a review I tried before {D80079} its really isCStyle() if we WERE going to pursue this, I'd want that and not have clauses added everywhere


================
Comment at: clang/tools/clang-format/ClangFormat.cpp:124
 
+static cl::opt<std::string>
+    SetLanguage("set-language",
----------------
Is this required for this patch or is this a nice to have, can we have a separate review for this, it may have merit in its own right but it needs it own tests


================
Comment at: clang/tools/clang-format/ClangFormat.cpp:467
 
+  if (SetLanguage.getNumOccurrences() != 0) {
+
----------------
I don't understand what you are doing here or why? is this just for C it feels wrong


================
Comment at: clang/tools/clang-format/ClangFormat.cpp:469
+
+    if (SetLanguage.getValue() == "C" || SetLanguage.getValue() == "c") {
+      FormatStyle->Language = FormatStyle::LK_C;
----------------
we don't use braces on single line if

wouldn't you use a tolower or toupper here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117416