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

resolve std name conflict #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

resolve std name conflict #2

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 3, 2016

Due to forward declarations and the structure of std includes I had compiler warning for redeclaring some atomic structures.
Here is a possible solution using defines and an internal namespace in std.

using rl::mo_release;
using rl::mo_acq_rel;
using rl::mo_seq_cst;
namespace rlimp {
Copy link
Owner

Choose a reason for hiding this comment

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

{ on new line please

@dvyukov
Copy link
Owner

dvyukov commented Jun 5, 2016

What compiler are you using? And what warnings does it produce?

@ghost
Copy link
Author

ghost commented Jun 6, 2016

I had some std header included (vector, string) and on Visual Studio 2015 it was complaining about the memory_order:
error C2874: using-declaration causes a multiple declaration of 'rl::memory_order'.
The include hierarchy was something like: vector included xmemory which included xatomic.

On GCC (5, the current mainstream gcc compiler on ubuntu) it had a similar problem with the atomic.
I have not checked that.

//# define atomic_intmax_t rlimp::atomic_intmax_t
//# define atomic_uintmax_t mutex rlimp::atomic_uintmax_t

# define mutex rlimp::mutex
Copy link
Owner

Choose a reason for hiding this comment

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

Does mutex lead to the same problems?
It's pretty common name, so redefining it is unfortunate...

@ghost
Copy link
Author

ghost commented Jun 6, 2016

It was a fast search/replace and did not considered any other name conflict.
I know it's not the best solution but had no better idea.
The best would be to pretend that atomic, thread, mutex, etc. has already been included, but I doubt if it has a solution independent of the implementation of libstd.

Note, I had the define problem with delete too.

class Foo {
  Foo(const Foo&) = delete;
}

@dvyukov
Copy link
Owner

dvyukov commented Jun 6, 2016

It was a fast search/replace and did not considered any other name conflict.

So does mutex lead to the same problems or not?

Foo(const Foo&) = delete;

Just don't use this with Relacy.
Define-based instrumentation sucks, it all needs to be redone to use compiler instrumentation. But I don't have time for that.

@ghost
Copy link
Author

ghost commented Jun 8, 2016

mutex was ok, did not have the problem.
Unfortunatelly I forget to create a branch before push/PR. So I'll close the PR and create a new branch.
Are u interested in the patch ? Shall I resend the PR from the new branch?

@dvyukov
Copy link
Owner

dvyukov commented Jun 9, 2016

Unfortunatelly I forget to create a branch before push/PR. So I'll close the PR and create a new branch.

git push -f?

Are u interested in the patch ?

Yes. It fixes build for you, right?
Without mutex/condvar changes it looks good to me.

Thanks

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.

1 participant