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

Namespace for globals? #1468

Closed
johnomotani opened this issue Dec 18, 2018 · 2 comments
Closed

Namespace for globals? #1468

johnomotani opened this issue Dec 18, 2018 · 2 comments
Labels
proposal A code/feature outline proposal

Comments

@johnomotani
Copy link
Contributor

Would it be a good idea to put the global variables defined in globals.hxx (mesh and dump) into a namespace like bout::globals?

I've come across bugs a few times caused by parts of the library using the global mesh when they should have been using a local one.

If we put using namespace bout::globals into physicsmodel.hxx then user code could use the global namespaces transparently and (once #1464 is merged) the using statement would not propagate into any library code except physicsmodel.hxx/cxx. This would help root out any remaining unintentional uses of the global variables in the library code, and places that we do want them (like default arguments) can use e.g. bout::globals::mesh to avoid any possibility of name conflicts.

@johnomotani johnomotani added the proposal A code/feature outline proposal label Dec 18, 2018
@d7919
Copy link
Member

d7919 commented Dec 18, 2018

We've been starting to use namespaces a bit more in recent PRs (see #1415 and the recent derivatives changes, for example) and I think it would be great to expand this effort.

As you've noted, by itself this could break code but there are ways to avoid this. I can think of (at least) two ways we could manage this in the interim:

  1. We namespace things in headers, try to compile the code (all tests and examples as well as library), find where it breaks, update to use the namespace, commit and then finally add a using ..... to the header. This would ensure user code would continue to work whilst allowing us to update the codebase to only use the namespaced routines. The downside of this is that it only catches non-namespaced use at one point in time -- we'd have to remain vigilant in PRs to ensure they don't sneak back in (harder than it sounds!) and there's also no obvious way to automatically alert users that something should be changed in their code to ensure it will continue to work in the future (when we remove the using statements).
  2. As with one except we add the using statements to a new header like bout_all.hxx (probably should just be bout.hxx, but this already exists). This has a possible advantage that user code wishing to use the library just has to include a single header (see Use more forward declarations #1134 for some comments on this). The downside is that this single header might be quite slow to include (as it would have to include all the headers that define the public api*). It might also be difficult to inform users of the change, however this change would break users models with the simple fix being just replace your includes with this magic include line.

@johnomotani
Copy link
Contributor Author

Implemented by #1470.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A code/feature outline proposal
Projects
None yet
Development

No branches or pull requests

2 participants