[cfe-commits] [PATCH] Improve handling of DLL import/export attributes

[João Matos]

On Wed, Sep 5, 2012 at 9:06 PM, Aaron Ballman <aaron at aaronballman.com>wrote:

> Yes and no.  I was thinking more along the lines of a way to turn off
> the warning one-off.  Sort of like how you can silence "unused
> variable" warnings using (void) or self-assignment.  It may not be
> possible to do this here, so at the very least we would need a warning
> group for it.  -Wmicrosoft-dll doesn't seem quite right to me, but I
> don't have a better suggestion either.
>

I changed it locally to use -Wmicrosoft-dll-attrs for these warnings. I see
now what you meant, well if there is any suggestion on a good syntax to do
it I can try do add it.

That would make sense, but I think that your patch doesn't address the
> underlying problem.
>
> void foo( void ) {
>         __declspec( dllimport ) __declspec( dllexport ) int x = 5;
> }
>
> void foo( void ) {
>         __declspec( dllexport ) __declspec( dllimport ) int x = 5;
> }
>
> These two examples produce different warnings.  The latter says that
> dllimport is ignored but then fails to ignore it, the former fails to
> tell you it's ignoring the dllimport.
>

Alright, thanks for the code, I'll check why this happens.

>> So I think the diagnostic should point to the current record's

> >> dllexport and then have a note pointing to the base class declaration
> >> tying the two together.  Also, I wonder if we could add a fixit to
> >> supply the dllexport for the definition of the base class?
> >
> >
> > Well a note is fine, but I think pointing to the base class points to the
> > real problem. Adding a note is fine but will be more verbose and the real
> > problem will be in the note and not the main diagnostic.
>
> The base class's declaration is mismatched with the declspec on the
> subclass, which is why I think it would make the most sense to point
> to the declspec and then note to the base class declaration.  This
> tells the user "these two things relate."


Oh I see what you mean, you mean pointing to the base's class attribute.
Alright, this makes more sense.


> Generally speaking, it's better to add test cases to an existing file
> than it is to add an entirely new file.  However, your approach here
> changes the nature of some of the tests which is a bad thing.  We need
> those tests to continue to work in C, so moving them to a C++ source
> file reduces the test coverage.  I would create a C++ file that has
> the C++-specific tests in it, but continue to add to the C test file
> cases which make sense.
>

Makes sense, I didn't consider that the C++ semantics could change the
meaning of the constructs in the C test code.


-- João Matos
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120905/b2d052eb/attachment.html>