MyDeveloperDay added inline comments.
================ Comment at: clang/lib/Format/ContinuationIndenter.cpp:1373 + else if (!Current.isOneOf(tok::comment, tok::identifier) && + !Keywords.isPPHash(Current, Style) && !Current.isStringLiteral()) State.StartOfStringLiteral = 0; ---------------- I'm not a fan of these isPPHash changes as I think it makes the impact on the other code much greater I think it would be better to handle the backtick case completely separately, in my experience as soon as you try to represent 2 things as one you are in trouble, my developer gut says there is something wrong here even if it works 99% of the time the last 1% will kill you. ================ Comment at: clang/lib/Format/FormatToken.h:723 + if (ForcedPrecedence != prec::Unknown) + return ForcedPrecedence; return getBinOpPrecedence(Tok.getKind(), /*GreaterThanIsOperator=*/true, ---------------- Could you show the unit test that covers this, does it impact C++ at all? ================ Comment at: clang/lib/Format/FormatToken.h:737 /// Returns the next token ignoring comments. - LLVM_NODISCARD const FormatToken *getNextNonComment() const { - const FormatToken *Tok = Next; + LLVM_NODISCARD FormatToken *getNextNonComment() const { + FormatToken *Tok = Next; ---------------- unrelated change ================ Comment at: clang/lib/Format/FormatToken.h:942 + KEYWORD(function, ATTR_JS_KEYWORD | ATTR_CSHARP_KEYWORD | \ + ATTR_VERILOG_KEYWORD | ATTR_VERILOG_HIER) \ KEYWORD(get, ATTR_JS_KEYWORD | ATTR_CSHARP_KEYWORD) \ ---------------- I feel like you are overloading a different level of classification here as to the "type of function" this just makes this structure now massive and hard to read. ================ Comment at: clang/lib/Format/FormatToken.h:1153 + KEYWORD(with, ATTR_VERILOG_KEYWORD) \ + KEYWORD(wor, ATTR_VERILOG_KEYWORD | ATTR_VERILOG_QUALIFIER) ---------------- What standard are we following here? for example I notice `noshowcancelled` is not covered.. despite me actually doing some VHDL during my degress, I no nothing about this, just want to understand what we should be covering. I guess alot of these words are not unit tested right? ================ Comment at: clang/lib/Format/FormatToken.h:1177 + ATTR_VERILOG_KEYWORD = 0x4, + ATTR_VERILOG_PP_DIRECTIVE = 0x8, + ATTR_VERILOG_END = 0x10, ---------------- this feels like mixed metaphors as I said above ================ Comment at: clang/lib/Format/FormatToken.h:1192 IdentifierInfo *kw_internal_ident_after_define; + IdentifierInfo *backtick; + IdentifierInfo *backtickbacktick; ---------------- I'm kind of wondering why this is needed (can you point me to the unit tests) because doesn't javascript already support this kind of interpolated string? ================ Comment at: clang/lib/Format/FormatToken.h:1324 + /// Returns whether \p Tok is a Verilog preprocessor directive. + bool isVerilogPPDirective(const FormatToken &Tok) const { + auto Info = Tok.Tok.getIdentifierInfo(); ---------------- Does this need to be VerilogSpecific? ================ Comment at: clang/lib/Format/FormatToken.h:1468 + return Tok.is(TT_GotoLabelColon) || + (Style.Language == FormatStyle::LK_Verilog && + Tok.is(tok::kw_default) && ---------------- ================ Comment at: clang/lib/Format/FormatTokenLexer.cpp:84 handleTemplateStrings(); - } - if (Style.Language == FormatStyle::LK_TextProto) + } else if (Style.Language == FormatStyle::LK_TextProto) { tryParsePythonComment(); ---------------- unrelated change commit separately to reduce patch size ================ Comment at: clang/lib/Format/FormatTokenLexer.cpp:471 + if (Next->isOneOf(tok::l_brace, tok::colon, tok::comment) || + Keywords.isPPHash(*Next, Style)) return false; ---------------- year really not nice.. its like we can never use tok::hash again! ================ Comment at: clang/lib/Format/FormatTokenLexer.cpp:513 +bool FormatTokenLexer::tryMergeTokensAny( + ArrayRef<ArrayRef<tok::TokenKind>> Kinds, TokenType NewType) { ---------------- This change could be pull out separately to reduce the size of this patch ================ Comment at: clang/lib/Format/FormatTokenLexer.cpp:929 +/// Count the length of leading whitespace in a token. +static size_t countLeadingWhitespace(StringRef Text) { + // Basically counting the length matched by this regex. ---------------- This is an unrelated change surely... and you point me to some unit tests for this ================ Comment at: clang/lib/Format/FormatTokenLexer.cpp:1175 +bool FormatTokenLexer::readRawTokenLanguageSpecific(Token &Tok) { + if (Style.Language != FormatStyle::LK_Verilog) ---------------- This is only for Verilog right? I think we should make that clear `readRawTokenVerilogSpecific` ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:736 CurrentToken->setType(TT_AttributeColon); - } else if (Left->isOneOf(TT_ArraySubscriptLSquare, + } else if (Style.isCpp() && + Left->isOneOf(TT_ArraySubscriptLSquare, ---------------- Could this impact any of the non C++ languages like C# ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:1195 case tok::comma: - if (Contexts.back().InCtorInitializer) + switch (Contexts.back().ContextType) { + case Context::CtorInitializer: ---------------- To ease the pain of such a massive patch you could have rolled this change in first, switching from the if's to a switch is a good change but you could massively reduce the impact of the patch by doing this in phases ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:1416 CurrentToken->is(Keywords.kw_package)) || - (Info && Info->getPPKeywordID() == tok::pp_import && - CurrentToken->Next && + ((Style.isCpp() || Style.Language == FormatStyle::LK_Proto || + Style.Language == FormatStyle::LK_Java) && ---------------- Did other languages creep in? ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:1869 + } else if (Current.isOneOf(tok::exclaim, tok::tilde) && + !((Current.TokenText == "not" || Current.TokenText == "compl") && + (!Current.getNextNonComment() || ---------------- is this something verilog specific? what impact does this have on https://en.cppreference.com/w/cpp/keyword/not ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:2001 + /// Test whether \p Tok is a qualifier like "const". + const FormatToken *qualifierToSkipBack(const FormatToken *Tok) { + // FIXME: Before I added this function, every language except ---------------- probably could be a separate patch ================ Comment at: clang/lib/Format/TokenAnnotator.h:46 LeadingEmptyLinesAffected(false), ChildrenAffected(false), + IsContinuation(Line.IsContinuation), FirstStartColumn(Line.FirstStartColumn) { ---------------- I feel this might be an unrelated change, is this something that needed for something specific, could you point to the unit tests 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