Page MenuHomePhabricator

Teach libc++ to use native NetBSD's max_align_t
ClosedPublic

Authored by krytarowski on Jun 6 2018, 2:32 AM.

Details

Summary

The NetBSD headers ship with max_align_t, that is not
compatible with the fallback version in libc++.

There is no defined a compiler specific symbol in the headers like:

  • __CLANG_MAX_ALIGN_T_DEFINED
  • _GCC_MAX_ALIGN_T
  • __DEFINED_max_align_t

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rCXX libc++

Event Timeline

krytarowski created this revision.Jun 6 2018, 2:32 AM
bsdjhb added a subscriber: bsdjhb.Jun 27 2018, 5:00 PM

FWIW, for FreeBSD I defined __CLANG_MAX_ALIGN_T and _GCC_MAX_ALIGN_T in FreeBSD's <stddef.h> when defining the typedef to handle this.

I saw this and I think that it shall be handled in libc++. NetBSD doesn't care if c++ runtime library is libstdc++, libc++, none or a different one.

It's similar to MUSL, __DEFINED_max_align_t is musl specific and they don't care about libc++ or not on top of it.

chandlerc resigned from this revision.Jul 6 2018, 6:54 PM

I don't have any problem with this patch's code in theory but:

  1. The question of whether this macro approach is better or worse than FreeBSD's or MUSL's seems a question for libc++ maintainers, not me.
  2. The question of whether this is correct for NetBSD seems for the NetBSD maintainer.

Since the above folks are already on the list of reviewers, I'll bow out of this review.

include/stddef.h
55

Unrelated to your patch, but this comment is now amazingly out of place.

If there are no more comments, I will land this by the end of this week.

joerg requested changes to this revision.Aug 20 2018, 1:10 PM
This revision now requires changes to proceed.Aug 20 2018, 1:10 PM
joerg accepted this revision.Aug 20 2018, 1:11 PM
This revision is now accepted and ready to land.Aug 20 2018, 1:11 PM

If there are no more comments, I will land this by the end of this week.

Just for the record, this is not OK and not how LLVM's code review works.

You can and must wait for review. I think Joerg already marked this as accepted, but it didn't send out an email[1]. That means it got reviewed, so all is good here, but I wanted to make it clear to others on the list what the expectations are here.

-Chandler

[1]: FYI, it's useful to put *some* text in the text box when marking a revision as accepted on Phab so that it will in fact send email. I typically put "LGTM" or a brief thank-you note to the author.

@chandlerc thank you for your note. I've originally treated this change as an obvious code change, just the reviewers regardless of reviewing an analogous change for MUSL, had no interest in NetBSD.

This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Aug 21 2018, 7:20 AM
include/stddef.h
55

Nope, based on git blame I think it's still where it should be.

(To be clear, this continues to not be related to this patch, but happy to discuss...)

include/stddef.h
55

I don't understand ... I'm not saying this patch has anything to do with it. But commenting *this* block with the above comment doesn't parse for me at all... Maybe its that the wording of the comment doesn't parse for me.

Perhaps what it means is "Only provide our own max_align_t (rather than relying on a compiler-provided one) when we have to by testing for the cases where the compiler's <stddef.h> won't work."?