Re: [PATCH 00/13] Patches for handling property attributes correctly in objc.

[Jean-Charles BERTIN]

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 <address@hidden> 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 address@hidden 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
> >> address@hidden
> >> 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
> > address@hidden
> > 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