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? > 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
