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

[David Chisnall]

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