Skip to content
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

Build failures due to template const member functions updating object fields #267

Closed
jmarshall opened this issue Jul 4, 2022 · 3 comments
Assignees
Labels

Comments

@jmarshall
Copy link

C++ compilers have always generally rejected classes with const member functions that modify non-mutable object fields. However they have historically accepted such code when it is in a templated class that is never instantiated or the const member function in question is never instantiated.

Clang (from v11 or so onwards) no longer accepts such code even if the const member function is never used or instantiated.

So the following in applications/bed/bedextract/src/ExtractRows.cpp causes a build failure with recent Clang:

  template <typename ValueType>
  struct Cache {
    Cache() : empty_(true)
      { }

    template <typename T>
    void operator()(const T* t) {
      empty_ = false;
    }

    bool Empty() const { return empty_; }
    void Clear() const { empty_ = true; }

  private:
    bool empty_;
  };

The Clear() method is currently never used, so the fix would be either to remove it or to make it non-const (hence void Clear() { empty_ = true; }) as it is clearly not really a const function.

A similar build failure is caused by interfaces/general-headers/utility/BitMonitor.hpp's BSet::next_set(), which attempts to update the _open private member field. BSet is used only by PooledCharMemory2 which is itself unused, so one fix would be to remove both classes. Alternatively, this may be an instance in which the BSet::_open field should be marked as mutable.

@alexpreynolds
Copy link
Collaborator

Patched in 2bd7994. Thanks for the report and to Shane Neph for additional feedback. I am running Clang v13.1.6 (well past v11), so I'm not sure why this didn't return any errors on my end. In any case, I'll close this up, and please feel free to follow up if you still run into errors.

@jmarshall
Copy link
Author

Apple Clang version numbers diverged from (and are inflated compared to) upstream LLVM version numbers years ago. Wikipedia's table suggests they have since converged somewhat, but testing this particular behaviour shows otherwise.

That change LGTM. Note that CI on this branch is failing because ubuntu-16.04 is long gone; suggest updating .github/workflows/main.yml to use ubuntu-latest instead.

@alexpreynolds
Copy link
Collaborator

Thanks for the Clang info, makes sense now. The action should be an easy change, thanks.

BiocondaBot pushed a commit to bioconda/bioconda-recipes that referenced this issue Jul 12, 2022
Merge PR #35647, commits were: 
 * Update to bedops 2.4.40 and reinstate macOS build

As described in omit-bad-mutable.patch, bedops contains code that
fails to compile with recent versions of Clang. Fortunately none
of the problematic code is actually used, so we can work around
the problem simply by removing it.

Reported upstream as bedops/bedops#267.
jpfeuffer pushed a commit to OpenMS/bioconda-recipes that referenced this issue Sep 7, 2022
Merge PR bioconda#35647, commits were: 
 * Update to bedops 2.4.40 and reinstate macOS build

As described in omit-bad-mutable.patch, bedops contains code that
fails to compile with recent versions of Clang. Fortunately none
of the problematic code is actually used, so we can work around
the problem simply by removing it.

Reported upstream as bedops/bedops#267.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants