logan added a comment.
Hi @timonvo,
I have committed this change as http://reviews.llvm.org/rL262178. Thanks for
your work.
Let's move forward to http://reviews.llvm.org/D15781.
http://reviews.llvm.org/D15883
___
cfe-commits mailing list
cfe-com
timonvo added a comment.
I don't have commit access. Can I land this CL myself somehow without commit
access (e.g. using arc), or will you have to submit it for me?
http://reviews.llvm.org/D15883
___
cfe-commits mailing list
cfe-commits@lists.llvm.
logan accepted this revision.
logan added a comment.
LGTM. It is good to go now.
@timonvo, do you have commit access? Or, do you need some assistance?
http://reviews.llvm.org/D15883
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://l
timonvo updated this revision to Diff 49215.
timonvo added a comment.
That's basically the Diff 48015 I had at some point. Reverted back to it now.
Now this patch doesn't declare any macros anymore.
http://reviews.llvm.org/D15883
Files:
lib/Headers/unwind.h
Index: lib/Headers/unwind.h
=
logan added inline comments.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
compnerd wrote:
> logan wrote:
> > logan wrote:
> > > compnerd wrote:
> > > > logan wrote:
> > > > > compnerd wrote:
> > > > > > logan wrote:
> > >
compnerd added inline comments.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
logan wrote:
> logan wrote:
> > compnerd wrote:
> > > logan wrote:
> > > > compnerd wrote:
> > > > > logan wrote:
> > > > > > Since this is `unwi
logan added inline comments.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
logan wrote:
> compnerd wrote:
> > logan wrote:
> > > compnerd wrote:
> > > > logan wrote:
> > > > > Since this is `unwind.h`, I feel that we can ge
logan added inline comments.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
compnerd wrote:
> logan wrote:
> > compnerd wrote:
> > > logan wrote:
> > > > Since this is `unwind.h`, I feel that we can get a step further and us
compnerd added inline comments.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
logan wrote:
> compnerd wrote:
> > logan wrote:
> > > Since this is `unwind.h`, I feel that we can get a step further and use
> > > `__ARM_EABI_
logan added inline comments.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
compnerd wrote:
> logan wrote:
> > Since this is `unwind.h`, I feel that we can get a step further and use
> > `__ARM_EABI_UNWINDER__` to get more
compnerd added a comment.
This change seems fine to me as is, just waiting to iron out the macro
situation with @logan before accepting it.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
+
logan wrote:
> Since this is `unwi
logan added a comment.
In general, it looks good to me if the comments are addressed.
Sorry for not replying responsively. Not sure why I missed the follow-up
e-mails after @rengolin's reply.
Comment at: lib/Headers/unwind.h:61
@@ +60,3 @@
+#define _UNWIND_ARM_EHABI 0
+#endif
timonvo added a comment.
Note that I avoided using a local macro at first because I didn't want to
export any additional symbols/macros from this file. But I've moved to use one
now. Note that I changed the name slightly to be more in line with libunwind's
naming (it uses _LIBUNWIND_ARM_EHABI a
timonvo updated this revision to Diff 48026.
timonvo added a comment.
Added local macro for enhanced readability.
http://reviews.llvm.org/D15883
Files:
lib/Headers/unwind.h
Index: lib/Headers/unwind.h
===
--- lib/Headers/unwind.
timonvo added a comment.
Hi compnerd, thanks for getting back to me. I've updated the code.
I was wondering though, where did you see @nbjoerg's comment? I couldn't find
any reference to it here or on cfe-commits.
http://reviews.llvm.org/D15883
__
timonvo updated this revision to Diff 48015.
timonvo added a comment.
Guard ARM EHABI enums using \#ifs (compnerd/nbjoerg's advice).
http://reviews.llvm.org/D15883
Files:
lib/Headers/unwind.h
Index: lib/Headers/unwind.h
===
---
compnerd added a comment.
It was on the mailing list, which won't show up on phabricator (email is still
the defacto review system).
Why not create a local macro and use that to make this easier to read?
#if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) &&
!defined(__ARM_DWARF_EH__
compnerd added a subscriber: compnerd.
compnerd requested changes to this revision.
compnerd added a reviewer: compnerd.
compnerd added a comment.
This revision now requires changes to proceed.
These should be guarded by a check to ensure that they are not defined when
EHABI is not in effect at @
timonvo added a comment.
Could someone please take a look at this patch as well as
http://reviews.llvm.org/D15781? Thanks!
http://reviews.llvm.org/D15883
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/
On Tue, Jan 05, 2016 at 05:27:34AM +, Timon Van Overveldt via cfe-commits
wrote:
> Adds a number of constants, defined in the ARM EHABI spec, to the Clang
> lib/Headers/unwind.h header. This is prerequisite for landing
> http://reviews.llvm.org/D15781, as previously discussed there.
IMO they
timonvo added a comment.
Logan, how should we proceed here? Can you approve the patch?
I also have no committer rights to the project, so does that mean you (or
someone else) need to commit it on my behalf?
http://reviews.llvm.org/D15883
___
cfe-c
timonvo added a comment.
In http://reviews.llvm.org/D15883#319445, @rengolin wrote:
> Tests?
Sure, I could add some. But is it common practice to test constants? The tests
would end up being a duplication of the definition, I'm not sure how valuable
that would be. I also couldn't find tests f
rengolin added a comment.
Well, I only saw later that these are propositions from another patch...
I don't see why this can't be part of the original patch, but I'm ok with no
tests if they're used (and tested) on the final patch.
I'll defer to Logan to decide. :)
http://reviews.llvm.org/D158
rengolin added a comment.
Tests?
http://reviews.llvm.org/D15883
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
24 matches
Mail list logo
