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

make static function inline in header #51

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

rob-p
Copy link
Contributor

@rob-p rob-p commented Nov 27, 2024

Clang warns:

util.hpp:127:17: warning: 'static' function 'get_seed_for_hash_function' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
  static uint64_t get_seed_for_hash_function(build_configuration const& build_config) {

I think this could cause potential issues across translation units and so it's probably best to inline this?

Clang warns:

```
util.hpp:127:17: warning: 'static' function 'get_seed_for_hash_function' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
  static uint64_t get_seed_for_hash_function(build_configuration const& build_config) {
```

I think this could cause potential issues across translation units and so it's probably best to inline this?
@jermp
Copy link
Owner

jermp commented Nov 27, 2024

Ok, thanks! I get no warning under clang 13.0, but not it's time to update clang probably...

@rob-p
Copy link
Contributor Author

rob-p commented Nov 27, 2024

This is coming from Apple clang 15 (on a GitHub CI runner).

@jermp
Copy link
Owner

jermp commented Nov 27, 2024

Right! Just updated to Clang to 14 and no warning there.
Setting up a CI workflow for SSHash is a very good idea. Do you have some suggestions?

@rob-p
Copy link
Contributor Author

rob-p commented Nov 27, 2024

Hi @jermp --- so there are several layers of CI one can set up. The simplest is just checking that the code builds on the intended target. That's fairly straightforward and we should be able to get that set up quickly.

Presumably, however, we probably want the CI to actually run some tests. I've done this with local CI before, but not much with GitHub actions. However, I think the idea would be that you build the artifact with one action, and then can make it available to other actions to run the tests. So we could have a test to e.g. build an index, issue queries, etc. Then, each action could pass or fail and you can gate the final production of a new release / publishing of a tag to the success of all of these actions.

Likewise, you can also run different workflows across different branches. So, perhaps, you only want the basic CI (i.e. it builds) to run on dev, while you want the full CI with all tests to run on main.

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