[llvm-commits] FW: [PATCH] enabling generation of ELF objects on Windows with the help of the triple

[Bendersky, Eli]

Thanks for the review -- my answers below:

> > Re-sending the patch itself (by request on IRC)
> I thought about this patch a little bit more. And I think we should have
> something "symmetric" wrt other targets/environments.
> 
> How do you feel about the following: we should have "sane" set of defaults
> (macho on darwin, coff on win, elf everywhere else). If one will explicitly ask
> for other format, it should be tolerated, regardless whether elf was asked on
> windows or macho. What do you think? I believe this will make the patch
> cleaner... I don't like special case of ELF everywhere, such cases tend to be
> forgotten in many places.
> 
<<from Michael Spencer>>
>I agree with Anton here. This should be consistent and not just special cased for ELF/Windows.
>
>The only thing I'm not sure about is how much effort will be required to properly respect the requested format. As it stands, there is quite a bit of >code that assumes Windows => COFF and Darwin => MachO. There are also features, such as TLS, which depend on the format, but are all >currently based on the OS.

Let me see if I grok your proposal. You want the "container override" to be implemented in a way that is generic and not only active for ELF. Is that right? In this case, I think we're already almost there. If you look at the usage of Triple.getEnvironment(), you'll notice that the "MachO override" was already implemented. What my patch adds is an override for ELF - which is, IIUC, exactly what you're asking for  - perhaps not coded in a sufficiently clear way and not adding the option of asking for ELF on Mac?

So what's left is "COFF override"? Do you propose to add an environment named "COFF" and decide upon that, too? Would this make sense even if no one is currently interested in this?

> Minor nitpicks:
> 
> -    return !isTargetDarwin() && !isTargetWindows() && !isTargetCygMing();
> +      return TargetTriple.getEnvironment() == Triple::ELF || (
> +          !isTargetDarwin() && !isTargetWindows() &&
> + !isTargetCygMing());
> Put ( at the new line :)
> 

Thank you, these will be fixed before committing.

Eli

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.