On Wed, Sep 5, 2012 at 1:19 AM, João Matos <[email protected]> wrote:
> Thanks for the review. Comments below. > > On Tue, Sep 4, 2012 at 7:47 PM, Aaron Ballman <[email protected]>wrote: >> >> It would be good to specify *what* type it is accessible from. Also, > > how would you "ignore" this warning if you wanted to (short of turning >> it off entirely)? >> > > I can add a new warning group for all of the DLL-related warnings > (-Wmicrosoft-dll?), would this address your concerns? > 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. 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. -- Matthieu > > >> If you're going to hoist things from the if, why not go the whole way? >> > > I didn't want to change much of the existing code but I can do that. > > >> You should probably also be checking the storage class specifier since >> dllimport must have external linkage. Eg) >> >> static __declspec( dllimport ) int l; // This is an error >> > > This check should be done in HandleDLLShared(). I'll add one more test > case for this. > > >> Why is dllimport preferred over dllexport? This is backwards from >> what MSDN documents the behavior to be >> (http://msdn.microsoft.com/en-us/library/twa2aw10). Also, the second >> if could be an else if to be more explicit that it can only be one or >> the other. >> > > When the code reaches this point it should only have one attribute. Maybe > I should add an assert / comment to make this clearer. > > >> Might want a "previously declared here" note to point out that this >> conflict is happening because of the class. >> > > Alright. > > >> When will this accessibility check be implemented? Since this is all >> new code, I'd rather not add the FIXME unless this is a major >> undertaking worthy of a separate patch. >> > > 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. > ^ > >> 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. > > 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. > > >> > + // "As a rule, everything that is accessible to the DLL's client >> (according >> > + // to C++ access rules) should be part of the exportable interface. >> > + // This includes private data members referenced in inline >> functions." >> > + // FIXME: Add more checks for those complex cases. >> >> Why not add those checks right now? Or at the very least, make the >> FIXME something actionable. What complex cases still need covering? >> > > Well I already covered some of this in the comment above. It seems to me > that friend functions make this harder than it seems. > > >> I wonder if this is a case for a fixit, since we could conceivably >> just add extern to the declaration and continue. >> > > 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. > > >> If this was a variable declaration, didn't you just set the linkage by >> setting the storage class on VD? >> > > Yeah, but this should catch the problem on other declaration kinds. > > >> > - // Attribute can be applied only to functions or variables. >> > - FunctionDecl *FD = dyn_cast<FunctionDecl>(D); >> > - if (!FD && !isa<VarDecl>(D)) { >> > - // Apparently Visual C++ thinks it is okay to not emit a warning >> > - // in this case, so only emit a warning when -fms-extensions is not >> > - // specified. >> >> That's because you can dllimport a class (at least you can according >> to http://msdn.microsoft.com/en-us/library/81h27t8c). I don't think >> this is a valid diagnostic. >> > > Indeed. But I am confused here, are you talking about the code that was > removed? > > I don't think this is valid either as it is explicitly allowed by MSVC >> (even though I think it's a bit strange): >> > > 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. > > >> Also, you are missing a check to ensure dllimport hasn't already been >> specified. Currently you can do __declspec( dllimport ) __declspec( >> dllimport ) int x; and it will not warn. >> > > Shouldn't that be checked by a more general duplicated attribute system? > If this is not true, I can add a new diagnostic. > > - // Attribute can be applied only to functions or variables. > >> > - FunctionDecl *FD = dyn_cast<FunctionDecl>(D); >> > - if (!FD && !isa<VarDecl>(D)) { >> >> Again, I don't think this is correct since you can dllexport a class. >> > > Again, are you commenting the removed code? > > > - S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) >> > - << Attr.getName() << 2 /*variable and function*/; >> > - return; >> > - } >> > - >> > // Currently, the dllexport attribute is ignored for inlined >> functions, unless >> > // the -fkeep-inline-functions flag has been used. Warning is >> emitted; >> > + // FIXME: MSDN says this should work for inline functions in all >> cases. >> >> Are you intending to address this FIXME? >> > > 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. > > >> > + FunctionDecl *FD = dyn_cast<FunctionDecl>(D); >> > if (FD && FD->isInlineSpecified()) { >> > // FIXME: ... unless the -fkeep-inline-functions flag has been >> used. >> >> Or this one? >> > > This was already here and it should be fixed when all the inline support > is reworked. > > Why are you getting rid of this entire file? If you have C++-specific >> tests to add for things like classes, then you should add a C++ file >> and put those tests there instead of getting rid of the entire C file. >> Also, it might be worth testing export/import structures from C. >> > > 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. > > >> I'd like to see more test cases as well, such as ones missing from >> MSDN, as well as edge cases like duplicate dllimport or dllexport >> attributes, etc >> > > Sure, these can be added. > > -- > João Matos > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
