On Jun 20, 2012, at 9:13 AM, Jordan Rose wrote: > On Jun 20, 2012, at 9:00 , Jordan Rose <[email protected]> wrote: >> On Jun 20, 2012, at 12:51 AM, Chandler Carruth wrote: >>>> On Mon, Jun 18, 2012 at 3:09 PM, Jordan Rose <[email protected]> wrote: >>>> Author: jrose >>>> Date: Mon Jun 18 17:09:19 2012 >>>> New Revision: 158683 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=158683&view=rev >>>> Log: >>>> Support -Winternal-linkage-in-inline in C++ code. >>>> >>>> This includes treating anonymous namespaces like internal linkage, and >>>> allowing >>>> const variables to be used even if internal. The whole thing's been broken >>>> out >>>> into a separate function to avoid nested ifs. >>>> >>> This warning seems interesting, and I generally like the correctness >>> insistence, but the current output is really unhelpful. Let's look at one >>> of the *many* cases I'm trying to fix in LLVM's own code: >>> >>> In file included from ../lib/Transforms/Utils/SSAUpdater.cpp:29: >>> ../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:406:54: warning: function >>> 'PHI_begin' is in an anonymous namespace but is used in an inline method >>> with external linkage [-Winternal-linkage-in-inline] >>> for (typename Traits::PHI_iterator I = Traits::PHI_begin(PHI), >>> ^ >>> >>> So, for starters this message doesn't read naturally with the code snippet >>> its pointing at because the message is a bit inverted. Imagine it instead >>> read as: >>> >>> "warning: calling function 'PHI_begin' from an inline method with external >>> linkage, but this method is defined in an anonymous namespace" >>> >>> This actually starts with the code under the cursor, and takes the user to >>> the nature of the problem. >> >> I see the ordering problem; there's a bit of a dangling reference here (does >> "this method" refer to PHI_begin or the current method?). I'll try to come >> up with something else. > > Since the action to take will almost certainly be to change the linkage of > the referenced function/variable/method, what do you think about showing the > declaration site at the warning and "used here" at the note? It makes the > message match the location. The downside is that the message is nowhere near > the /use/, so if it's something like Xcode's warnings you'd have to jump to > the note to see why this triggered.
This warning really concerns me for a lot of reasons, not least of which being that changing the linkage of the reference function/variable/method is *not* necessarily the appropriate action; in many cases the appropriate fix might be to change the linkage of the referring code. More importantly, though, while I understand the instinct of wanting to diagnose violations of the ODR, it is overwhelmingly likely that this is an innocuous violation. The only way it's *not* innocuous is if we really care about it being the same thing across translation units, e.g. if we're taking its address and expecting it to compare equal, or if it's a mutable variable, or something like that. For example, the minLinkage function that Chandler fixed was indeed technically an ODR violation, but there's no *bug* there. There are zero negative consequences to having called this trivially-inlined function from code that's potentially shared across translation units. I do not see why we should be interested in pedantically diagnosing violations of the ODR that have no negative consequences for our users. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
