<br><br><div class="gmail_quote">On Wed, Sep 5, 2012 at 1:19 AM, Joćo Matos <span dir="ltr"><<a href="mailto:ripzonetriton@gmail.com" target="_blank">ripzonetriton@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote">Thanks for the review. Comments below.</div><div class="gmail_quote"><br></div><div class="gmail_quote"><div class="im">On Tue, Sep 4, 2012 at 7:47 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It would be good to specify *what* type it is accessible from. Also,</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
how would you "ignore" this warning if you wanted to (short of turning<br>
it off entirely)?<br></blockquote><div><br></div></div><div>I can add a new warning group for all of the DLL-related warnings (-Wmicrosoft-dll?), would this address your concerns?</div></div></blockquote><div><br>For most warnings there is a note (or a few notes) explaining the changes to make into the code so that the warning no longer fires.<br>
<br>For example, if you write if (x = 4) you get a warning and two notes, the notes suggest to either change = into == OR to add parentheses around x = 4.<br><br>-- Matthieu<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If you're going to hoist things from the if, why not go the whole way?<br></blockquote><div><br></div></div><div>I didn't want to change much of the existing code but I can do that.</div><div class="im"><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
You should probably also be checking the storage class specifier since<br>
dllimport must have external linkage. Eg)<br>
<br>
static __declspec( dllimport ) int l; // This is an error<br></blockquote><div><br></div></div><div>This check should be done in HandleDLLShared(). I'll add one more test case for this.</div><div class="im"><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Why is dllimport preferred over dllexport? This is backwards from<br>
what MSDN documents the behavior to be<br>
(<a href="http://msdn.microsoft.com/en-us/library/twa2aw10" target="_blank">http://msdn.microsoft.com/en-us/library/twa2aw10</a>). Also, the second<br>
if could be an else if to be more explicit that it can only be one or<br>
the other.<br></blockquote><div><br></div></div><div>When the code reaches this point it should only have one attribute. Maybe I should add an assert / comment to make this clearer.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>Might want a "previously declared here" note to point out that this</div>
conflict is happening because of the class.<br></blockquote><div><br></div></div><div>Alright.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
When will this accessibility check be implemented? Since this is all<br>
new code, I'd rather not add the FIXME unless this is a major<br>
undertaking worthy of a separate patch.<br></blockquote><div><br></div></div><div>I'm not sure about this. It seems to me a lot more complex than it first appears. What if a friend function or class that is exported accesses some of the private class members? Then you also need to export it, and it seems pretty hard to this check in a fast way. For now I just export everything. I commented about this on the bug tracker where I first posted the patch and asked for some opinions about this, but no one gave any suggestions, so I suggest we try to optimize this later.</div>
<div class="im">
<div> ^</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So I think the diagnostic should point to the current record's<br>
dllexport and then have a note pointing to the base class declaration<br>
tying the two together. Also, I wonder if we could add a fixit to<br>
supply the dllexport for the definition of the base class?<br></blockquote><div><br></div></div><div>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. </div>
<div><br></div><div>I thought about the fix it, but this sometimes happens for system types like the STL and MS's stance about it has been to just ignore it, I was worried that the fix it system might try to fix the system headers.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + // "As a rule, everything that is accessible to the DLL's client (according<br>
> + // to C++ access rules) should be part of the exportable interface.<br>
> + // This includes private data members referenced in inline functions."<br>
> + // FIXME: Add more checks for those complex cases.<br>
<br>
Why not add those checks right now? Or at the very least, make the<br>
FIXME something actionable. What complex cases still need covering?<br></blockquote><div><br></div></div><div>Well I already covered some of this in the comment above. It seems to me that friend functions make this harder than it seems.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I wonder if this is a case for a fixit, since we could conceivably<br>
just add extern to the declaration and continue.<br></blockquote><div><br></div></div><div>Sure I thought of adding a fixit, but thought it could lead to it doing the wrong thing in some cases where the attribute is badly applied. I don't think it's worth it to add a fix it here.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If this was a variable declaration, didn't you just set the linkage by<br>
setting the storage class on VD?<br></blockquote><div><br></div></div><div>Yeah, but this should catch the problem on other declaration kinds.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> - // Attribute can be applied only to functions or variables.<br>
> - FunctionDecl *FD = dyn_cast<FunctionDecl>(D);<br>
> - if (!FD && !isa<VarDecl>(D)) {<br>
> - // Apparently Visual C++ thinks it is okay to not emit a warning<br>
> - // in this case, so only emit a warning when -fms-extensions is not<br>
> - // specified.<br>
<br>
That's because you can dllimport a class (at least you can according<br>
to <a href="http://msdn.microsoft.com/en-us/library/81h27t8c" target="_blank">http://msdn.microsoft.com/en-us/library/81h27t8c</a>). I don't think<br>
this is a valid diagnostic.<br></blockquote><div><br></div></div><div>Indeed. But I am confused here, are you talking about the code that was removed?</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I don't think this is valid either as it is explicitly allowed by MSVC<br>
(even though I think it's a bit strange):<br></blockquote><div><br></div></div><div>Yes, I know this is supported, but I'm not trying to support yet with this patch. Selective members and inline / friends referencing private data will come later.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, you are missing a check to ensure dllimport hasn't already been<br>
specified. Currently you can do __declspec( dllimport ) __declspec(<br>
dllimport ) int x; and it will not warn.<br></blockquote><div><br></div></div><div>Shouldn't that be checked by a more general duplicated attribute system? If this is not true, I can add a new diagnostic.</div><div class="im">
<div> > - // Attribute can be applied only to functions or variables.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> - FunctionDecl *FD = dyn_cast<FunctionDecl>(D);<br>
> - if (!FD && !isa<VarDecl>(D)) {<br>
<br>
Again, I don't think this is correct since you can dllexport a class.<br></blockquote><div><br></div></div><div>Again, are you commenting the removed code?</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> - S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)<br>
> - << Attr.getName() << 2 /*variable and function*/;<br>
> - return;<br>
> - }<br>
> -<br>
> // Currently, the dllexport attribute is ignored for inlined functions, unless<br>
> // the -fkeep-inline-functions flag has been used. Warning is emitted;<br>
> + // FIXME: MSDN says this should work for inline functions in all cases.<br>
<br>
Are you intending to address this FIXME?<br></blockquote><div><br></div></div><div>Not at the moment, there is much more important stuff to fix in the MS support. But eventually I'd like to tackle these weirder cases.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + FunctionDecl *FD = dyn_cast<FunctionDecl>(D);<br>
> if (FD && FD->isInlineSpecified()) {<br>
> // FIXME: ... unless the -fkeep-inline-functions flag has been used.<br>
<br>
Or this one?<br></blockquote><div><br></div></div><div>This was already here and it should be fixed when all the inline support is reworked.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Why are you getting rid of this entire file? If you have C++-specific<br>
tests to add for things like classes, then you should add a C++ file<br>
and put those tests there instead of getting rid of the entire C file.<br>
Also, it might be worth testing export/import structures from C.<br></blockquote><div><br></div></div><div>That was actually how I had it, but I remember reading someone asking to keep the number of test cases down (I think it was Chandler, and this was not to me specifically) so I merged it into one.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'd like to see more test cases as well, such as ones missing from<br>
MSDN, as well as edge cases like duplicate dllimport or dllexport<br>
attributes, etc<br></blockquote><div><br></div></div><div>Sure, these can be added.</div><span class="HOEnZb"><font color="#888888"><div><br></div></font></span></div><span class="HOEnZb"><font color="#888888">-- <br>Joćo Matos<br>
</font></span><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br>