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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
