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