[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

This revision was automatically updated to reflect the committed changes. Closed by commit rG0066a09579ca: [libc++] Give extern templates default visibility on gcc (authored by smeenai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35388/new/

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

ldionne removed a reviewer: EricWF. ldionne added a subscriber: EricWF. ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. In D35388#2493696 , @smeenai wrote: > It's been a long time since I've

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

smeenai added a comment. It's been a long time since I've contributed to libc++, and the pre-merge CI setup is a massive improvement over what we had before. A huge kudos to everyone who made it possible :) This is still showing up as Needs Review. I'm not sure if that's because of @EricWF's

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

smeenai updated this revision to Diff 316161. smeenai added a comment. Herald added a project: libc++. Herald added a subscriber: libcxx-commits. Herald added a reviewer: libc++. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35388/new/

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

ldionne accepted this revision. ldionne added a comment. Can you rebase onto `main` and re-upload the diff? It will trigger CI. If the CI passes, this is good to go. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35388/new/

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

thomasanderson added a comment. We appear to still be carrying a patch for this in Chromium, so I think the issue still stands. https://source.chromium.org/chromium/chromium/src/+/master:buildtools/third_party/libc++/BUILD.gn;drc=21272efa27e69622c6d174f29e4a73f1a6088cfc;l=135 Repository: rG

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

smeenai added a comment. Sorry, I seem to have somehow blanked out on this diff entirely. Is it still relevant? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35388/new/ https://reviews.llvm.org/D35388

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

thakis added a comment. Herald added a subscriber: dexonsmith. > I would think that both this change and https://reviews.llvm.org/D35326 are > good. What's the status here and in D35326 ? We still have a workaround in chromium's build files pointing at these

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

ldionne added a comment. In https://reviews.llvm.org/D35388#1167315, @thomasanderson wrote: > In https://reviews.llvm.org/D35388#1167305, @ldionne wrote: > > > ABI-wise, I think this change is OK. Indeed, if `__type_visibility__` is > > not supported, we're already marking the base template as

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

thomasanderson added a comment. In https://reviews.llvm.org/D35388#1167305, @ldionne wrote: > ABI-wise, I think this change is OK. Indeed, if `__type_visibility__` is not > supported, we're already marking the base template as > `__visibility__("default")`, so marking the extern template

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

ldionne added a comment. ABI-wise, I think this change is OK. Indeed, if `__type_visibility__` is not supported, we're already marking the base template as `__visibility__("default")`, so marking the extern template declaration with `__visibility__("default")` is not a problem. If

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

EricWF added a reviewer: ldionne. EricWF added a subscriber: ldionne. EricWF added a comment. Adding @ldionne as an observer, since he's knee-deep in the visibility mess right now. After re-evaluating, I think I was being overly cautious the last time I looked at this. I think this patch

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

smeenai added a comment. Filed https://bugs.llvm.org/show_bug.cgi?id=34614 about the silent attribute ignoring. https://reviews.llvm.org/D35388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

smeenai added a comment. Good point. It looks like clang actually ignores the attributes in that case as well; it just doesn't warn you about it :D Take a look at https://godbolt.org/g/CJTD6c to see what I mean. Note the `.hidden c::f()` in both the gcc and clang outputs.

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

EricWF requested changes to this revision. EricWF added a comment. This revision now requires changes to proceed. Hmm. So the documentation is wrong, but not "fully wrong". If you simply try applying this patch and compiling with GCC you'll still encounter a bunch of instances of `warning: type

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

thomasanderson added a comment. Hm.. is there anyone else who might be able to review? https://reviews.llvm.org/D35388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

thomasanderson added a comment. Friendly ping for @EricWF https://reviews.llvm.org/D35388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35388: [libc++] Give extern templates default visibility on gcc

smeenai created this revision. Contrary to the current visibility macro documentation, it appears that gcc does handle visibility attribute on extern templates correctly, e.g. https://godbolt.org/g/EejuV7. We need this so that extern template instantiations of classes not marked