On 04/01/2014 22:40, Aaron Ballman wrote:
On Sat, Jan 4, 2014 at 4:55 PM, Alp Toker <[email protected]> wrote:
Hi Aaron,
The identical-looking blocks look like a straight code-move so I haven't
examined them closely (if there are any significant changes in there, do
mention). Here's the rest..
You have it correct -- the code simply moved around, but has no
substantive changes.
+// Defines targets for target-specific attributes. The list of strings
should
+// specify architectures for which the target applies, based off the
ArchType
+// enumeration in Triple.h.
+class TargetArch<list<string> arches> {
+ list<string> Arches = arches;
+ list<string> OSes;
+}
+def TargetARM : TargetArch<["arm", "thumb"]>;
+def TargetMSP430 : TargetArch<["msp430"]>;
+def TargetX86 : TargetArch<["x86"]>;
+def TargetX86Win : TargetArch<["x86", "x86_64"]> {
+ let OSes = ["Win32", "MinGW32"];
+}
+def TargetMips : TargetArch<["mips", "mipsel"]>;
+
Feels like it'd be more TableGen-ish to apply TargetArch<> directly to the
instances without listing these utility wrappers. Maybe only provide the
defs for TargetMips and TargetARM that add value? It's your call though if
you prefer to keep it this way -- I don't have a strong opinion here.
I don't have a strong opinion either way, but I have a slight
preference for being more declarative like the patch is doing. The
rationale being: as people add more target-specific attributes, these
will be reused, which makes it easier to modify the targets (such as
adding thumb to arm, etc).
Quite reasonable, on balance I'm fine with this.
-def DLLExport : InheritableAttr, TargetSpecificAttr {
+def DLLExport : InheritableAttr, TargetSpecificAttr<TargetX86Win> {
let Spellings = [Declspec<"dllexport">];
let Subjects = SubjectList<[Function, Var]>;
}
-def DLLImport : InheritableAttr, TargetSpecificAttr {
+def DLLImport : InheritableAttr, TargetSpecificAttr<TargetX86Win> {
let Spellings = [Declspec<"dllimport">];
// Technically, the subjects for DllImport are Function and Var, but
there is
// custom semantic handling required when MicrosoftExt is true.
Any reason for dllimport / dllexport to be x86 target attributes?
Because they were x86 previously. This patch was going for feature
parity with the existing functionality.
That quirk
looks like a historical leftover that can be handled by the ["Win32",
"MinGW32"] predicate alone thanks to your changes. MSDN documents them as
being general language features not specific to x86 and I believe they apply
to other Microsoft architectures equally.
That I could definitely believe, but should probably be a separate
patch. Good catch, though!
Fair enough. I'm looking forward to that cleanup as well.
LGTM
Alp.
~Aaron
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
