erichkeane marked 14 inline comments as done.
erichkeane added a comment.
Incoming patch!
================
Comment at: include/clang/Basic/Attr.td:1809
bool DuplicateArchitecture = false;
+ bool operator ==(const ParsedTargetAttr &Other) {
+ return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
rsmith wrote:
> Our normal convention is to not put a space before `==` here.
Clang format seems to keep adding it interestingly enough. Perhaps because it
is a .td file? Will remove.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9317-9318
+def err_target_required_in_redecl :
+ Error<"function declaration is missing 'target' attribute in a "
+ "multiversioned function">;
+def note_multiversioning_caused_here :
----------------
rsmith wrote:
> There's some weird indentation hereabouts. Our normal convention is to put
> the `Error<` on the `def` line, and start the diagnostic text as a
> 2-space-indented string literal on the next line.
Thanks! Clang-format really makes a mess of these too...
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2144
+ if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
+ StringRef MangledName = getMangledName(CurFD);
+ llvm::Constant *Func = GetGlobalValue(MangledName);
----------------
rsmith wrote:
> You should skip functions you've already seen somewhere around here; we don't
> need to handle the same function multiple times.
Will the lookup call give me duplicates? I need to check each at least once
through this so that I can put them into "Options" so that I can emit them
during the resolver.
Otherwise, I'm not terribly sure what you mean.
================
Comment at: lib/Sema/SemaDecl.cpp:9326-9328
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl
*OldFD,
+ const FunctionDecl *NewFD,
+ bool CausesMV) {
----------------
rsmith wrote:
> rsmith wrote:
> > Would it be possible to factor out the checks in `MergeFunctionDecl` that
> > you're using here and reuse them directly?
> Maybe also check that the language linkage matches (`extern "C"` and `extern
> "C++"` could imply different calling conventions, even though they don't on
> any of our current targets).
I'd thought about that, but I'm being WAAY more strict, so I didn't see a good
way to factor them out without making things pretty complicated. Suggestions
welcome :)
================
Comment at: lib/Sema/SemaDecl.cpp:9383-9384
+
+ QualType OldReturnType = OldType->getReturnType();
+ QualType NewReturnType = NewType->getReturnType();
+ if (OldReturnType != NewReturnType) {
----------------
rsmith wrote:
> This won't do the right thing for C++14 deduced return types. I'm not
> completely sure what the right thing is, though -- clearly we need to deduce
> the same return type in all versions of the function, but it's less clear
> whether we should also require the return-type-as-written to match across
> versions (as is required across redeclarations). Example:
>
> ```
> VERSION_FOO auto f() { return 0; } // declared return type is `auto`, actual
> return type is `int`
> VERSION_BAR auto f(); // will fail the current check, because the return type
> `auto` doesn't match the prior return type `int`
> VERSION_BAR auto f() { return 0.0; } // should reject this because we deduced
> a different return type than on the VERSION_FOO version?
> ```
>
> Perhaps the simplest thing would be to simply disallow deduced return types
> for multiversioned functions entirely for now?
I think disallowing 'auto' return for now is a good idea. This next patch is
using isUndeducedType, which I think is sufficient, but if there is a better
function I'm missing, please let me know.
https://reviews.llvm.org/D40819
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
