aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:237 + std::string Val = HNOption.General.lookup(Opt.first); + if (Val.empty()) { + HNOption.General.insert({Opt.first, Opt.second.str()}); ---------------- Usual style is to elide braces for single-line `if` statements (applies elsewhere in the patch as well). ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:254 + static constexpr std::pair<StringRef, StringRef> CStrings[] = { + {"char*", "sz"}, {"char", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t", "wsz"}}; + for (const auto &CStr : CStrings) { ---------------- One thing that confused me was the plain `char` and `wchar_t` entries -- those are for arrays of `char` and `wchar_t`, aren't they? Can we use `char[]` to make that more clear? If not, can you add a comment to clarify? ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:376-380 + static constexpr std::pair<StringRef, StringRef> HNCStrings[] = { + {"CharPrinter", "char*"}, + {"CharArray", "char"}, + {"WideCharPrinter", "wchar_t*"}, + {"WideCharArray", "wchar_t"}}; ---------------- Similar question here as above, but less pressing because we at least have the word "array" nearby. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:421-422 + std::string OptionVal = StrMap.lookup(OptionKey); + for (auto &C : OptionVal) + C = toupper(C); + ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:431 static IdentifierNamingCheck::FileStyle -getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options) { +getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options, + IdentifierNamingCheck::HungarianNotationOption &HNOption) { ---------------- Formatting ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:591-592 + for (const auto &Type : HNOption.PrimitiveType) { + std::string Key = Type.getKey().str(); + if (ModifiedTypeName == Key) { + PrefixStr = Type.getValue(); ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:602-603 + for (const auto &Type : HNOption.UserDefinedType) { + std::string Key = Type.getKey().str(); + if (ModifiedTypeName == Key) { + PrefixStr = Type.getValue(); ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:667 + std::string Initial; + for (auto const &Word : Words) { + Initial += tolower(Word[0]); ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:678-680 + if (clang::Decl::Kind::EnumConstant == ND->getKind() || + clang::Decl::Kind::CXXMethod == ND->getKind() || + clang::Decl::Kind::Function == ND->getKind()) { ---------------- No need to check for `CXXMethodDecl` because those inherit from `FunctionDecl` anyway. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:694 + // character instead of the `getEndLoc()` function. + char *EOL = const_cast<char *>(strchr(Begin, '\n')); + if (!EOL) { ---------------- I think `EOL` should probably be `const char *` and can remove remove all these `const_cast<>`? ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:781 + std::string Prefix; + switch (ND->getKind()) { + case clang::Decl::Kind::Var: ---------------- I think this `switch` should be replaced with: ``` if (const auto *ECD = dyn_cast<EnumConstantDecl>(ND)) { ... } else if (const auto *CRD = dyn_cast<CXXRecordDecl>(ND)) { ... } else if (isa<VarDecl, FieldDecl, RecordDecl>(ND)) { ... } ``` ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:786 + case clang::Decl::Kind::Record: + if (ND) { + std::string TypeName = getDeclTypeName(ND); ---------------- Can remove all of the `if (ND)` in this method -- we already early return if `ND == nullptr`. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:826-827 + for (const auto &Str : Map) { + bool Found = (Str.getValue() == CorrectName); + if (Found) { + Words.assign(Words.begin() + 1, Words.end()); ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:874 +static std::string +fixupWithCase(const StringRef &Type, const StringRef &Name, const Decl *D, + const IdentifierNamingCheck::NamingStyle &Style, ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1038 static std::string -fixupWithStyle(StringRef Name, - const IdentifierNamingCheck::NamingStyle &Style) { - const std::string Fixed = fixupWithCase( - Name, Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase)); +fixupWithStyle(const StringRef &Type, const StringRef &Name, + const IdentifierNamingCheck::NamingStyle &Style, ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1348 static llvm::Optional<RenamerClangTidyCheck::FailureInfo> getFailureInfo( - StringRef Name, SourceLocation Location, + const StringRef &Type, const StringRef &Name, const NamedDecl *ND, + SourceLocation Location, ---------------- There's no need to pass a `StringRef` by const reference -- they pass like a pointer type already. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1389 + return getFailureInfo( + TypeName, Decl->getName(), Decl, Loc, FileStyle.getStyles(), HNOption, + findStyleKind(Decl, FileStyle.getStyles(), FileStyle.isIgnoringMainLikeFunction()), SM, ---------------- And can remove the declaration of `TypeName` above. Also, can you correct the formatting issues, elsewhere as well. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:125 + + IdentifierNamingCheck::HungarianNotationOption HNOption; }; ---------------- Might as well clear up this lint warning. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:55 return std::string(llvm::formatv( - "Add using-declaration for {0} and remove qualifier.", Name)); + "Add using-declaration for {0} and remove qualifier", Name)); } ---------------- This change looks unrelated to the patch? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:363 std::string title() const override { - return "Move function body to out-of-line."; + return "Move function body to out-of-line"; } ---------------- Same here? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:41 Expected<Effect> apply(const Selection &Inputs) override; - std::string title() const override; + std::string title() const override { + return "Remove using namespace, re-qualify names instead"; ---------------- And these changes as well? ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:110 + + When set to `true` the check will ensure the name will add Hungarian + Notation prefix for the given data type. The default value is `Off`. ---------------- For all of these, I'd recommend a slight wording tweak to: ``` When enabled, the check ensures that the declared identifier will have a Hungarian notation prefix based on the declared type. ``` ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2264-2265 + +This tool supports all casing types that means not only CamelCase starts with +Hungarian Notation, others either. + ---------------- I would remove this paragraph as it doesn't really add much value (it effectively says that the option works well with other options, but that's already expected). ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2312 +**HungarianNotation.CString.*** + Options for NULL terminated string, you can customized your prefix here. + ---------------- ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2315 +**HungarianNotation.DerivedType.*** + Options for derived types, you can customized your prefix here. + ---------------- ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2318 +**HungarianNotation.PrimitiveType.*** + Options for primitive types, you can customized your prefix here. + ---------------- ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2321-2322 +**HungarianNotation.UserDefinedType.*** + Options for redefined types(Microsoft data types), you can customized your + prefix here. + ---------------- ================ Comment at: lldb/include/lldb/Target/Process.h:565 + /// This does not change the pid of underlying process. + lldb::pid_t GetID() const { return m_pid; } + ---------------- All the changes in this file are unrelated. ================ Comment at: lldb/source/Target/Process.cpp:532 const UnixSignalsSP &unix_signals_sp) - : ProcessProperties(this), UserID(LLDB_INVALID_PROCESS_ID), + : ProcessProperties(this), Broadcaster((target_sp->GetDebugger().GetBroadcasterManager()), ---------------- Unrelated changes. ================ Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:167 if (isTypeLegal(VT)) { + setOperationAction(ISD::ABS, VT, Legal); + ---------------- All the changes in this file are unrelated. ================ Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.h:54 - // Integer absolute. - IABS, - ---------------- Unrelated changes. ================ Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.td:835 let CCValues = 0xF, CompareZeroCCMask = 0x8 in { - def LPR : UnaryRR <"lpr", 0x10, z_iabs, GR32, GR32>; - def LPGR : UnaryRRE<"lpgr", 0xB900, z_iabs, GR64, GR64>; + def LPR : UnaryRR <"lpr", 0x10, abs, GR32, GR32>; + def LPGR : UnaryRRE<"lpgr", 0xB900, abs, GR64, GR64>; ---------------- All the changes in this file are unrelated. ================ Comment at: llvm/lib/Target/SystemZ/SystemZInstrVector.td:574 def VLP : UnaryVRRaGeneric<"vlp", 0xE7DF>; - def VLPB : UnaryVRRa<"vlpb", 0xE7DF, z_viabs8, v128b, v128b, 0>; - def VLPH : UnaryVRRa<"vlph", 0xE7DF, z_viabs16, v128h, v128h, 1>; - def VLPF : UnaryVRRa<"vlpf", 0xE7DF, z_viabs32, v128f, v128f, 2>; - def VLPG : UnaryVRRa<"vlpg", 0xE7DF, z_viabs64, v128g, v128g, 3>; + def VLPB : UnaryVRRa<"vlpb", 0xE7DF, abs, v128b, v128b, 0>; + def VLPH : UnaryVRRa<"vlph", 0xE7DF, abs, v128h, v128h, 1>; ---------------- Unrelated changes. ================ Comment at: llvm/lib/Target/SystemZ/SystemZOperators.td:669 // Negative integer absolute. -def z_inegabs : PatFrag<(ops node:$src), (ineg (z_iabs node:$src))>; - -// Integer absolute, matching the canonical form generated by DAGCombiner. -def z_iabs32 : PatFrag<(ops node:$src), - (xor (add node:$src, (sra node:$src, (i32 31))), - (sra node:$src, (i32 31)))>; -def z_iabs64 : PatFrag<(ops node:$src), - (xor (add node:$src, (sra node:$src, (i32 63))), - (sra node:$src, (i32 63)))>; -def z_inegabs32 : PatFrag<(ops node:$src), (ineg (z_iabs32 node:$src))>; -def z_inegabs64 : PatFrag<(ops node:$src), (ineg (z_iabs64 node:$src))>; +def z_inegabs : PatFrag<(ops node:$src), (ineg (abs node:$src))>; ---------------- All the changes in this file are unrelated. ================ Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:15 #include "llvm/Transforms/Scalar/ConstraintElimination.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" ---------------- All the changes in this file are unrelated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86671/new/ https://reviews.llvm.org/D86671 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits