-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir] Silence -Wdangling-assignment-gsl in OperationSupport.h #126140
Conversation
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Shoaib Meenai (smeenai) ChangesThis warning is causing lots of build spam when I use a recent Clang as Full diff: https://github.com/llvm/llvm-project/pull/126140.diff 1 Files Affected:
diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h
index d4035d14ab74650..bcbf9458ff96e7f 100644
--- a/mlir/include/mlir/IR/OperationSupport.h
+++ b/mlir/include/mlir/IR/OperationSupport.h
@@ -997,13 +997,15 @@ struct OperationState {
if (!properties) {
T *p = new T{};
properties = p;
- propertiesDeleter = [](OpaqueProperties prop) {
+ constexpr auto deleter = [](OpaqueProperties prop) {
delete prop.as<const T *>();
};
- propertiesSetter = [](OpaqueProperties new_prop,
- const OpaqueProperties prop) {
- *new_prop.as<T *>() = *prop.as<const T *>();
+ propertiesDeleter = deleter;
+ constexpr auto setter = [](OpaqueProperties newProp,
+ const OpaqueProperties prop) {
+ *newProp.as<T *>() = *prop.as<const T *>();
};
+ propertiesSetter = setter;
propertiesId = TypeID::get<T>();
}
assert(propertiesId == TypeID::get<T>() && "Inconsistent properties");
|
This feels like a false positive, but does the error persist if you add a + before the lambda to force conversion to a function pointer? |
That was the first thing I tried. The warning was still there, and I actually got ASAN failures after making that change, which I wasn't expecting at all. |
If this is a false positive: isn't this a bug to report to clang instead of working around it like this? |
Per a3b4d91:
I'll post a Clang issue to clarify, but this generates a huge amount of build spam, so I think it's worth addressing that even if it's not the ultimate fix. |
Can we disable the warning for this build target instead? With a comment and a link to the clang issue for reeenabling when fixed? That's less intrusive that code changes that we won't track and revert. |
I posted #126600 to ask about the warning. I'll try silencing the warning instead here. |
This warning is causing lots of build spam when I use a recent Clang as my host compiler. It's a potential false positive, so silence it until llvm#126600 is resolved.
4e37d7b
to
265d190
Compare
@joker-eph I'm silencing the warning instead now, does it look okay to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/16752 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/1708 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/509 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/518 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/10112 Here is the relevant piece of the build log for the reference
|
Someone looking into this and has a fix? Otherwise I'd appreciate a revert. |
Sorry, I pushed a revert. |
Thank you, I appreciate it. |
…126140) This warning is causing lots of build spam when I use a recent Clang as my host compiler. It's a potential false positive, so silence it until llvm#126600 is resolved. Fix variable casing while I'm here.
…126140) This warning is causing lots of build spam when I use a recent Clang as my host compiler. It's a potential false positive, so silence it until llvm#126600 is resolved. Fix variable casing while I'm here.
llvm#126140)" This reverts commit f6556af. Buildbots are broken.
…126140) This warning is causing lots of build spam when I use a recent Clang as my host compiler. It's a potential false positive, so silence it until llvm#126600 is resolved. Fix variable casing while I'm here.
llvm#126140)" This reverts commit f6556af. Buildbots are broken.
…126140) This warning is causing lots of build spam when I use a recent Clang as my host compiler. It's a potential false positive, so silence it until llvm#126600 is resolved. Fix variable casing while I'm here.
llvm#126140)" This reverts commit f6556af. Buildbots are broken.
This warning is causing lots of build spam when I use a recent Clang as
my host compiler. It's a potential false positive, so silence it until
#126600 is resolved.
Fix variable casing while I'm here.