-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix has_static_new #1013
Fix has_static_new #1013
Conversation
.gitignore
Outdated
@@ -45,3 +45,5 @@ autowiring.kdev4 | |||
autowiring.VC.db | |||
.vscode | |||
AutowiringVersion.h | |||
|
|||
*.VC.db* |
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.
`Autowiring.VC.db is already listed above though?
Or if the intent here is the trailing asterisk, are there circumstances when it makes a db1, db2 file?
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.
Yeah, 2017 adds several extensions.
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.
I see now. I have Browse.VC.db
and Solution.VC.db{,-shm,-wal}
.
Should remove the now-redundant autowiring.VC.db
above then?
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.
yep
|
||
TEST_F(AutoConstructTest, MultiStaticNewTest) { | ||
AutoConstruct<MultiStaticNew> badstatic{ 1 }; | ||
} |
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.
missing newline at EOF
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.
This var is named badstatic
. However, it looks like the good one?
src/autowiring/has_static_new.h
Outdated
/// </summary> | ||
/// <remarks> | ||
/// A factory New routine is malformed when the return type is not implicitly castable to type T | ||
/// </remarks> |
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.
Should keep the comments? I found the <remarks>
line useful initially.
This implementation is effectively |
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.
I'm in agreement with @jdonald. Well done, @yeswalrus. If you can resolve @jdonald's remaining minor issues, this should be good to go.
[**] | ||
indent_style = space | ||
indent_size = 2 | ||
insert_final_newline = true |
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.
Ironic that this file is missing its final newline!
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.
Sadly, VS2017 has default support for editor config but ignores the 2 most useful options: insert_final_newline and trim_trailing_whitespace
MSVC2015, alone among MSVC versions and for reasons that are beyond me chokes on the function name detection portion of the old has_static_new's SFINAE.
This overhauls it to be far more concise, and compile correctly.