[BUG] Default constructor provided but header states should be deleted

[SeanAlling-DojoFive]

Description

In file.h constructor for input_file() has the comment

  /**
    * The default constructor.  Do not use.
    */
  input_file();

In file.cc, an implementation is provided:

srecord::input_file::input_file() :
    file_name("standard input"),
    line_number(1),
    prev_was_newline(false),
    vfp(stdin),
    checksum(0),
    ignore_checksums(ignore_checksums_default)
{
}

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:

  /**
    * The default constructor.  Do not use.
    */
  input_file() = delete;

If a default constructor is provided now, a compile error will occur.

[SeanAlling-DojoFive]

Option 2 is preferred, but we need to decide if C++11 should be used for project moving forward. Can create a PR for either instance.

[sierrafoxtrot]

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.

[SeanAlling-DojoFive]

I can go through and any constructor that is marked with do not use I can explicitly mark with = delete.

This way if other instances can be removed if found.

Along this vein of using C++11, should we also use = default? This would eliminate constructors that are simply {}.

I can take ownership of both.

[jtxa]

In #26 I did apply all automatic fixes for discussion with @sierrafoxtrot (that PR itself will not be merged). On most things there is an agreement, and many of them are fixed in #32 recently.

These 3 should be auto-fixed for better prototypes which includes your suggestions:

modernize-use-equals-default
modernize-use-equals-delete
modernize-use-override

But I suggest to do these beforehand:

cppcoreguidelines-pro-type-member-init
modernize-use-default-member-init
readability-redundant-member-init
readability-redundant-string-init

I hope that this way, almost all constructors will be =default. And only those remain, that really does something additionally.

Would be great if you want to take a look at those fixes. As those fixes are related and automated, I guess 1 or 2 commits are the right granularity. I don't know how the formatting will look like, perhaps a simple clang-format is needed to avoid manual reformatting. I created #42 which might help, AFAIK clang-tidy does only reformat changed lines, so it should be nice enough for these changes.

C++11 is definitely needed since replacing boost::shared_ptr by std::shared_ptr. It should be a good compromise for portability and modernity.

[sierrafoxtrot]

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.

[SeanAlling-DojoFive]

Since we are moving to using more C++11 constructors, another clang-tidy rule to use is, cppcoreguidelines-special-member-functions, which only warns and will not fix if a class is missing one of the 5 constructors in C++11.

Since I am already working on this PR, I think it makes sense to add missing constructors.

[SeanAlling-DojoFive]

Completed with merge of #43