This is an archive of the discontinued LLVM Phabricator instance.

Permit redeclarations of a builtin to specify calling convention.
ClosedPublic

Authored by erichkeane on Mar 19 2019, 1:53 PM.

Details

Summary

After https://reviews.llvm.org/rL355317 we noticed that quite a decent
amount of code redeclares builtins (memcpy in particular, I believe
reduced from an MSVC header) with a calling convention specified.
This gets particularly troublesome when the user specifies a new
'default' calling convention on the command line.

When looking to add a diagnostic for this case, it was noticed that we
had 3 other diagnostics that differed only slightly. This patch ALSO
unifies those under a 'select'. Unfortunately, the order of words in
ONE of these diagnostics was reversed ("'thiscall' calling convention"
vs "calling convention 'thiscall'"), so this patch also standardizes on
the former.

Diff Detail

Repository
rC Clang

Event Timeline

erichkeane created this revision.Mar 19 2019, 1:53 PM
rnk added inline comments.Mar 19 2019, 2:14 PM
lib/Sema/SemaDecl.cpp
3145

You can make these self-documenting with an enum, and then reference the enum name in a comment in the .td file. It seems the existing convention is to put these types of enums directly into the Sema class.

erichkeane marked an inline comment as done.Mar 19 2019, 2:15 PM
erichkeane added inline comments.
lib/Sema/SemaDecl.cpp
3145

Ah, great! I didn't realize there was a convention for how to do these.

As @rnk suggested, switch to an enum. SemaType.cpp unfortunately has Sema as a forward declaration, so it has to use integers.

Woops! Look like I left without actually changing the revision *shame*.

Also, I was an idiot about SemaType, please disregard my previous comment.

rnk accepted this revision.Mar 20 2019, 3:05 PM

lgtm

This revision is now accepted and ready to land.Mar 20 2019, 3:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 6:32 AM
Herald added a subscriber: kristina. · View Herald Transcript