-
Notifications
You must be signed in to change notification settings - Fork 23
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
[BUG] Default constructor provided but header states should be deleted #41
Comments
Option 2 is preferred, but we need to decide if |
Hi Sean. Nice find! I'm not sure how that one crept in but my best guess is that it was the original way that a stdin infile object was being instantiated. I'd need to go spelunking through Peter's early work to be sure. For a little background (but probably not a huge surprise), the intent of the declaration with "do not use" comment was to prevent accidental usage of a default constructor i.e. declare it but don't define it and compiler errors will save us from a hefty debug session. This pattern is replicated EVERYWHERE in the code. We have, thanks to @jtxa just moved to C++11 as a minimum as part of his clean-up using clang-tidy. So both options are available. I'm happy to just remove this instance but not opposed to option 2 as it might be a nice modernisation. The main thing is that I'd rather be consistent. So a PR to remove this one OR a PR that addresses the same issue (but also addresses the default destructors, copy constructors and assignment operators) across the codebase would be most welcome. |
I can go through and any constructor that is marked with This way if other instances can be removed if found. Along this vein of using C++11, should we also use I can take ownership of both. |
In #26 I did apply all automatic fixes for discussion with @sierrafoxtrot (that PR itself will not be merged). These 3 should be auto-fixed for better prototypes which includes your suggestions:
But I suggest to do these beforehand:
I hope that this way, almost all constructors will be Would be great if you want to take a look at those fixes. C++11 is definitely needed since replacing |
Sounds like a sound approach gents and @SeanAlling-DojoFive the help is greatly appreciated! Out of curiosity (and while we are in the neighbourhood), is there a comparable and more modern approach for the default constructors, copy constructors and assignment operators? If there is an obvious solution, we could at some point remove all the "Do not use" comments and apply a better solution. |
Since we are moving to using more C++11 constructors, another clang-tidy rule to use is, Since I am already working on this PR, I think it makes sense to add missing constructors. |
Completed with merge of #43 |
Description
In file.h constructor for
input_file()
has the commentIn file.cc, an implementation is provided:
Resolution
Two options to resolve issue:
Pre C++11
Remove the default constructor from file.cc
Post C++11
Remove the default constructor from file.cc
and change default constructor in file.h to:
If a default constructor is provided now, a compile error will occur.
The text was updated successfully, but these errors were encountered: