[Lldb-commits] [PATCH] D117564: [lldb] Remove the requirement for windows clients to specify -DIMPORT_LIBLLDB

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D117564#3254798 <https://reviews.llvm.org/D117564#3254798>, @thakis wrote:

>> So what happens now if you define LLDB_IN_LIBLLDB? Does dllexport explode if used in a static library?
>
> Do you mean define it for all targets, instead of for just liblldb?
>
> If just for liblldb, then the link fails because (e.g.) driver tries to dllimport the symbols in liblldb then.
>
> If you mean "define for everything", I haven't checked (since it's inconvenient to do).

I see. Thanks for explaining.

> To be clear, this isn't a huge problem for me, I'll just switch to a dll on Windows. The old system just seemed more flexible than the new setup, without being more complicated. (Maybe you're planning to do some followup that requires this change.)

The background here is that I found this patch in one of my old WIP branches. The next patch in the series was going to define LLDB_API to `__attribute__((visibility("default")))` on the non-windows path (and make everything else hidden), but I have no idea why I wanted to do that. I mean, it does not sounds like an *un*reasonable thing to do, but am not going to do unless I find some benefit to it (maybe it saves size?)

This patch, otoh, seemed useful (and it still does) on its own as it removes the need for windows users to define some extra macros (for the supported configuration, anyway).

Anyway, what would you say if we wrapped this entire LLDB_API-setting block in an `#ifndef LLDB_API`? That way, things would still work out-of-the-box in the default (supported) configuration, but users who know what they're doing can define it to visibility(default), nothing, or whatever else, and that would take precedence.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117564/new/

https://reviews.llvm.org/D117564