BTW, I introduced objc_property_clean_abi attribute because I need to know if clang is able to generate the new ABI. I agree that we also need a main switch (ie. prepocessor macro) when compiling libobjc2 to generate the right stuff. We will then be able to generate old ABI even if clang have the objc_property_clean_abi attribute.
Regards. On Wed, 2013-02-27 at 18:16 +0000, David Chisnall wrote: > I'm trying to review this, but it's impossible. Most of the changes are > unexplained, or the explanations are just wrong. For example, changing the > version from 1.7 to 1.8 (that won't happen until 1.7 is released). Resetting > the CXX_FLAGS and C_FLAGS in the test is completely wrong, you should be > appending -UNDEBUG, not clearing all of the user-specified flags: your patch > there just broke cross compiles. > > I'm really worried about your C coding ability when you think three lines of > pragmas to silence a warning is better than the three tokens required to fix > the code (i.e. add an explicit cast via intptr_t). > > The same applies to your changes in block_to_imp.c. mkstemp() may modify the > pattern, but that doesn't mean that you have to copy it. We want to use the > modified version in the next call. That's the entire point. > > I can't work out what you are trying to do with the properties code, because > you seem to be making it all conditionally compiled. The runtime is expected > to support code compiled with any version of clang, not have different > behaviour depending on the version of the compiler that compiled it. > > I don't know what the objc_property_clean_abi attribute is. You seem to have > spammed the cfe-commits list too. Please put clang patches into the LLVM > phabricator review system. At least one of them will definitely be rejected > (adding a default: to a switch statement where the switch already covers all > of the cases: LLVM intentionally doesn't do this because doing so silences a > compiler warning when you add a new value to the enumeration. In fact, when > compiling with clang, you will have just introduced a new warning). > > In short... please slow down. You seem to have made a huge number of changes > all over the place before asking for review. A lot of these changes were in > serious need of review, to prevent you from making the same mistakes > repeatedly. Some of these look like they may be real bug fixes, but they're > so tangled up in completely unexplained and often wrong changes that it's > very difficult to see if they really are. > > If you want to change the ABI for the runtime, then I'd suggest that you > email me first with your proposal. When targeting version 1.7 of the runtime > (-fobjc-runtime=gnustep-1.7), clang already provides property introspection > metadata in a format that is compatible with both Apple and older versions of > the runtime. > > David > > On 27 Feb 2013, at 17:18, Jean-Charles BERTIN <jc.ber...@axinoe.com> wrote: > > > I re-post my patches as a big one patch. > > > > On Wed, 2013-02-27 at 17:54 +0100, Jean-Charles BERTIN wrote: > >> These are my patches to change ABI for property attributes on Objective-C. > >> Actually, the behavior is clearly not the same as MacOS X. So this is my > >> effort to correct this. These patches are strongly correlated with these I > >> pushed on cfe-comm...@cs.uiuc.edu mailing list. > >> > >> Jean-Charles BERTIN (13): > >> Remove compile flags for all optimized targets because they usually > >> contain -DNDEBUG flag which disable assert() macro. > >> Removed RTTI information to avoid undefined _ZTIN4llvm10ModulePassE > >> symbol. > >> InlineCostAnalyzer was renamed to InlineCostAnalysis in LLVM 3.3. > >> Fixed ABI for objc property attributes to mimic MacOS X behavior. > >> Fixed ProtocolCreation test for new property attributes ABI. > >> Report failure for PropertyAttributesTest2 if compiler cannot generate > >> new property ABI. > >> Bumped library version. > >> Added more tests. > >> Fixed property_copyAttributeList() and > >> initPropertyFromAttributesList() to behave more like MacOS X. > >> Minor cleanup. > >> Bumped _XOPEN_SOURCE to include stpcpy() definition. > >> Removed hard coded PAGE_SIZE definition. > >> Fixed mkstemp() usage. > >> > >> _______________________________________________ > >> Gnustep-dev mailing list > >> Gnustep-dev@gnu.org > >> https://lists.gnu.org/mailman/listinfo/gnustep-dev > > > > -- > > Jean-Charles BERTIN > > Axinoe - Software Engineer > > Tel.: (+33) (0)1.80.82.59.23 > > Fax : (+33) (0)1.80.82.59.29 > > Skype: jcbertin > > Web: <http://www.axinoe.com/> > > Certificate Authority: <https://ca.axinoe.com/axinoe-root.crt> > > <big.patch>_______________________________________________ > > Gnustep-dev mailing list > > Gnustep-dev@gnu.org > > https://lists.gnu.org/mailman/listinfo/gnustep-dev > > > > > -- Sent from my brain > -- Jean-Charles BERTIN Axinoe - Software Engineer Tel.: (+33) (0)1.80.82.59.23 Fax : (+33) (0)1.80.82.59.29 Skype: jcbertin Web: <http://www.axinoe.com/> Certificate Authority: <https://ca.axinoe.com/axinoe-root.crt>
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev