Skip to content

Remove hidden open warning from vlsv_reader_parallel.h#63

Open
iljah wants to merge 2 commits into
masterfrom
iljah-patch-5
Open

Remove hidden open warning from vlsv_reader_parallel.h#63
iljah wants to merge 2 commits into
masterfrom
iljah-patch-5

Conversation

@iljah

@iljah iljah commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

GCC 16.1.1 gives:

In file included from vlsv_reader_parallel.h:24,
                 from vlsv_reader_parallel.cpp:25:
vlsv_reader.h:46:20: warning: ”virtual bool vlsv::Reader::open(const std::string&)” was hidden [-Woverloaded-virtual=]
   46 |       virtual bool open(const std::string& fname);
      |                    ^~~~
vlsv_reader_parallel.h:45:12: note:   by ”bool vlsv::ParallelReader::open(const std::string&, MPI_Comm, const int&, MPI_Info)”
   45 |       bool open(const std::string& fname,MPI_Comm comm,const int& masterRank,MPI_Info mpiInfo=MPI_INFO_NULL);
      |            ^~~~

Comment thread vlsv_reader_parallel.h Outdated
uint64_t getBytesRead();
double getReadTime() const;
bool getUniqueAttributeValues(const std::string& tagName,const std::string& attribName,std::set<std::string>& output) const;
using Reader::open;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves a comment in to code why it's there (and how it works). Just shutting up warnings so they are gone is only half the goal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens is mentioned in https://en.cppreference.com/cpp/language/virtual but https://stackoverflow.com/a/4146525 explains it better, I'll let someone else decide how to handle this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments to both instances

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants