aaron.ballman added inline comments.
================ Comment at: include/clang/Basic/Attr.td:1809 bool DuplicateArchitecture = false; + bool operator==(const ParsedTargetAttr &Other) { + return DuplicateArchitecture == Other.DuplicateArchitecture && ---------------- This function should be marked `const`. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9333-9335 + "multiversion function would have identical mangling to a previous " + "definition. Duplicate declarations must have identical target attribute " + "values">; ---------------- Diagnostics are not complete sentences, so this should be reworded to be even less grammatically correct. ;-) ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9339-9340 +def err_multiversion_no_other_attrs : Error< + "attribute 'target' multiversioning cannot be combined with other " + "attributes">; +def err_multiversion_diff : Error< ---------------- This worries me slightly. Is there a benefit to prohibiting this attribute with any other attribute? For instance, I'm thinking about a multiversioned noreturn function or a multiversioned function with a calling convention as plausible use cases. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9349 +def err_multiversion_not_allowed_on_main : Error< + "function multiversion not permitted on 'main'">; + ---------------- How about `'main' cannot be a multiversion function`? ================ Comment at: lib/CodeGen/CodeGenFunction.cpp:2307-2308 + SmallVector<StringRef, 8> FeatureList; + std::for_each(std::begin(RO.ParsedAttribute.Features), + std::end(RO.ParsedAttribute.Features), + [&FeatureList](const std::string &Feature) { ---------------- You can use `llvm::for_each` instead. ================ Comment at: lib/CodeGen/CodeGenFunction.h:3941 + Priority = std::max(Priority, TargInfo.multiVersionSortPriority( + StringRef{Feat}.substr(1))); + ---------------- `Feat` is already a `StringRef`? ================ Comment at: lib/CodeGen/CodeGenModule.cpp:741 + const auto &Target = CGM.getTarget(); + auto CmpFunc = [&Target](StringRef LHS, StringRef RHS) { + return Target.multiVersionSortPriority(LHS) > ---------------- Might as well sink this into the call to `std::sort()`. ================ Comment at: lib/Sema/SemaDecl.cpp:9298 + TargetAttr::ParsedTargetAttr ParseInfo = TA->parse(); + const auto &TargetInfo = S.Context.getTargetInfo(); + enum ErrType { Feature = 0, Architecture = 1 }; ---------------- Don't use `auto` here as the type is not explicitly spelled out in the initialization. ================ Comment at: lib/Sema/SemaDecl.cpp:9326 + +static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl *OldFD, + const FunctionDecl *NewFD, ---------------- This function uses quite a few magic numbers that might be better expressed with named values instead. ================ Comment at: lib/Sema/SemaDecl.cpp:9362-9364 + QualType NewQType = S.getASTContext().getCanonicalType(NewFD->getType()); + const FunctionType *NewType = cast<FunctionType>(NewQType); + QualType NewReturnType = NewType->getReturnType(); ---------------- Formatting is off here. ================ Comment at: lib/Sema/SemaDecl.cpp:9363 + QualType NewQType = S.getASTContext().getCanonicalType(NewFD->getType()); + const FunctionType *NewType = cast<FunctionType>(NewQType); + QualType NewReturnType = NewType->getReturnType(); ---------------- Can use `const auto *` here. ================ Comment at: lib/Sema/SemaDecl.cpp:9381 + QualType OldQType = S.getASTContext().getCanonicalType(OldFD->getType()); + const FunctionType *OldType = cast<FunctionType>(OldQType); + FunctionType::ExtInfo OldTypeInfo = OldType->getExtInfo(); ---------------- Can use `const auto *` here. ================ Comment at: lib/Sema/SemaDecl.cpp:9448 + if (NewFD->isMain()) { + if (NewTA && NewTA->getFeaturesStr() == "default") { + S.Diag(NewFD->getLocation(), diag::err_multiversion_not_allowed_on_main); ---------------- So any other form of target attribute and feature string is fine to place on `main()`? ================ Comment at: lib/Sema/SemaDecl.cpp:9478 + + auto *OldFD = OldDecl->getAsFunction(); + // Unresolved 'using' statements (the other way OldDecl can be not a function) ---------------- Do not use `auto` here. ================ Comment at: lib/Sema/SemaDecl.cpp:9494 + TargetAttr::ParsedTargetAttr NewParsed = NewTA->parse(); + // sort order doesn't matter, it just needs to be consistent. + std::sort(NewParsed.Features.begin(), NewParsed.Features.end()); ---------------- Capitalization. ================ Comment at: lib/Sema/SemaDecl.cpp:9562 + for (NamedDecl *ND : Previous) { + auto *CurFD = ND->getAsFunction(); + if (!CurFD) ---------------- Do not use `auto` here. ================ Comment at: lib/Sema/SemaDecl.cpp:9577 + TargetAttr::ParsedTargetAttr CurParsed = CurTA->parse(); + std::sort(CurParsed.Features.begin(), CurParsed.Features.end()); + ---------------- Given how often the features need to be sorted, would it make sense to hoist this functionality into `parse()`? https://reviews.llvm.org/D40819 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits