On Tue, Sep 4, 2012 at 7:19 PM, 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?
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. >> 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. 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. >> >> 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. Okay, that's fair. >> 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." > 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. We already have ways to ignore system header issues. >> 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. That is true -- there are two ways to fix this, one is to apply the extern the other is to remove the declspec. So nevermind, fixit likely won't help 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. Yes, it will. But it's still odd (and inefficient) to set the storage class then turn around and ask for it again. >> > - // 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? Sorry for the confusion, yes, I was talking about code that was removed. Oops! >> 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. It should be somewhere -- I think it would make more sense to check for duplicates in a generic way, yes. >> 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. 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. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
