- User Since
- Jan 18 2013, 11:30 AM (274 w, 3 d)
Sat, Apr 21
This does not address my review. My review is suggesting that we avoid this issue completely by fixing IRGen to use an external linkage for internal declarations in your emission mode. That would allow you to just emit the module ctors as truly internal in the first place, removing any need to mangle them.
These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those.
Wed, Apr 18
Yes, I'm sorry, I think you're right. I had misunderstood the language problem when I suggested going down this road.
Let me try to summarize where I think we are.
The goal of having a unified option is that you can uniformly suppress warnings that don't always make sense in unit tests. It's future-proofing against the addition of other warnings (probably C++ warnings) that might not make sense for unit tests, like extending the x &= x warning (which is not in -Wself-assign) to user-defined operators. You don't think you would be able to take advantage of that? Because -Wno-self-assign-overloaded-nonfield is rather, uh, pretty precisely targeted for exactly that use case; I can't imagine why someone would use -Wself-assign-overloaded-nofield *positively*, or even *negatively* for any purpose besides suppressing a unit-test problem.
I'm sorry, that warning *is* in self-assign, although I'm not sure it should be.
Mon, Apr 16
Thank you. Minor request and then LGTM.
Sat, Apr 14
If so, LGTM.
Fri, Apr 13
I'd still prefer if someone with more driver-design expertise weighed in, but we might not have any specialists there.
Sat, Apr 7
I think there was a point when we weren't always creating CXXThisExprs eagerly for these accesses. Now that we are, yeah, this should be dead.
Well, that is a really silly bug. Fix LGTM.
Committed as r329508.
Hmm. Alright, I guess.
Fri, Apr 6
If that's the problem, then I think the right design is for CallArg to include an optional cleanup reference for a cleanup that can be deactivated at the instant of the call (we should assert that this exists for parameters that are destroyed in the callee), and then for forwarding it's just a matter of getting that cleanup from parameter emission to forwarding-argument emission.
Just a couple minor requests; if you accept them, feel free to commit.
This looks okay to me, but I think it would better if someone with more expertise in the design of the driver and frontend code could review this.
Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass.
Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller?
Thu, Apr 5
Changing the requirements on the return-value slot would be an ABI break, no? And it would force to us to treat all complete-object constructions as nvsize-limited unless they're constructions of known sizeof-sized allocations.
The change LGTM, but please add a test.
Wed, Apr 4
It's not unreasonable to just add -fmerge-all-constants to the command line invocations for the individual tests, yeah. We can take those off as necessary later.
Thanks for splitting the patch up. LGTM.
Tue, Apr 3
The other changes I see here seem reasonable, but please do split the patch.
Okay, LGTM with the reduced set of changes to the functionality.
Okay. LGTM. Thank you for putting the effort into maintaining both rules simultaneously.
Yes, if we think that the committee is likely to include questionable functions on the [[nodiscard]] list which we don't want to warn about pre-C++17, then it makes sense to have two internal macros, one for functions like std::move that should be unconditionally warned about and one for the iffy-bu-standard-directed cases.
I think the appropriate place to do this is in IsStandardConversion, immediately after the call to ResolveAddressOfOverloadedFunction. You might want to add a general utility for getting the type-of-reference of a function decl.
Is Marshall arguing that the standard doesn't allow compilers to warn about failing to use these function results prior to C++17? Because I don't think that's true; warnings are thoroughly non-normative.
Trust me, I understand that this is an important function, but a lot of functions are important, and we're not going to hardcode knowledge about all of them in the compiler.
Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper, but he might consider taking a patch in the meantime. But I'm not sure what the rush is if it doesn't change what goes into any concrete release of the compiler + library.
Mon, Apr 2
No, the analysis is intentionally syntactic; it should apply even on dependently-typed arguments (but not get re-checked on instantiation). But I agree that the warning ought to be disabled in unevaluated contexts.
That seems reasonable. And this summer would still be in time for the next release.
Yeah, actually, I'm second-guessing myself. Maybe this should just be a libc++ / libstdc++ bug.
LGTM. I think it wouldn't be unreasonable to ask standard library maintainers to add [[nodiscard]] to std::move, but it's also not unreasonable for us to special-case some functions.
Right. I think it's fair to acknowledge that many data structure unit tests will contain a legitimate use of a user-defined self-assignment without feeling that that disqualifies the warning.
Sun, Apr 1
Sat, Mar 31
Thanks. LGTM whenever you've cleared up the self-hosting problems.
Fri, Mar 30
Seems fine to me, but you might want someone with analyzer experience to review.
Thu, Mar 29
What would the design for that warning be? C promotes all arithmetic on sub-int types to int, and if that's assigned back to a variable of the original type, there's a conversion. But you seem to only want to warn about truncating the result of multiplication and not, say, addition or negation. Is there a principle to this? Just the likelihood of escaping the range of the original type?
Wed, Mar 28
I think it's part of an effort to avoid creating implicit declarations for all the special members of every struct we parse from system headers.
Wow, the IR improvements here are amazing.
LGTM. I guess it just didn't matter for Objective-C because the variable always has to be an Objective-C pointer and the extra store of nil never hurt anything.
You should send an RFC to cfe-dev about adding this new language mode. I understand that it's very similar to an existing language mode that we already support, and that's definitely we'll consider, but we shouldn't just agree to add new language modes in patch review.
What exactly are you trying to express here? Are you just trying to make these external declarations when compiling for the device because __shared__ variables are actually defined on the host? That should be handled by the frontend by setting up the AST so that these declarations are not definitions.
If __global__ is supported in C++ structures, you might also need to make sure that member function constants (&A::kernel_function) drop the CC. And it might be a good idea to make sure that decltype(kernel_function) doesn't have a problem with it, either, since that does do some special-case work.
Tue, Mar 27
Is it possible to just do this for all structs? I don't think it hurts anything to do it for structs that are trivial and returned indirectly, and it would certainly be nice to construct C return values in-place even if they're guilty of nothing more than being, y'know, really really big.