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

[Jean-Charles BERTIN]

OK I admit I did that too quickly but I have push my changes on github a
long time ago...
What is the point to make a github account if nobody reviews the pull
requests? Since I saw no feedbacks from github I decided to push my
patches on the mailing list: I did that by pushing every single git
commit as a patch to discuss them one-by-one. I'm ok with the reject of
some of them since I'm really new with GNUstep (but not with objc on
MacOS X).

The main goal of this stuff is to fix compatibilty problems I found
when I tried to compile a code which works flawlessly on MacOS but
fails with GNUstep. I make a complete test case in
Test/PropertyAttributeTest2.m which can be compiled on both GNUstep and
MacOS (clang -o PropertyAttributeTest2 -framework Foundation
Test/PropertyAttributeTest2.m should do it on MacOS)
If you make a try of this test case with a vanilla GNUstep you will see
that every test fail on GNUstep but work on MacOS.
That's why I tried to fix that without breaking existing code so I
introduced the objc_property_clean_abi in clang and bumped the GNUstep
runtime version (I really missed that GNUstep runtime 1.7 was not yet
released)

I really apologize if I make some mistakes but I don't really understand
how the mkstemp stuff can work: the first time you invoke mkstemp(),
tmpPattern is modified and then contains a unique random path that you
use for mapping the first trampoline block. The next time, mkstemp()
will return the same unique random path that you use for mapping the
next trampoline block. This works because 1) the first time you unlink
the file that was opened 2) the next time mkstemp() will return an
another file descriptor that will map an another trampoline block. Am I
right?

Another question: which patch to you talk about pragmas for fixing a
warning?

I hope you will take a deeper look at my commits because I really think
that property attributes stuff is broken in regards to MacOS.

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