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

Headers are getting polluted by using namespace std #139

Closed
mrexodia opened this issue Oct 31, 2022 · 6 comments · Fixed by #143
Closed

Headers are getting polluted by using namespace std #139

mrexodia opened this issue Oct 31, 2022 · 6 comments · Fixed by #143
Assignees

Comments

@mrexodia
Copy link
Contributor

mrexodia commented Oct 31, 2022

The ghidra source code is riddled with using namespace std in various header files. This is bad practice and it would be nice if this could be patched away. It causes all kinds of confusing issues (one notable one being with std::byte and byte from some Windows COM headers).

Ideally these occurrences would be patched out, but obviously the cost is high because you'd have to maintain an extensive patch set...

@tetsuo-cpp
Copy link
Contributor

tetsuo-cpp commented Oct 31, 2022

Thanks for reporting @mrexodia.

Yeah, that's a bit of a pain...

Just off the top of my head, maybe we can have a test file that includes the "super" header libsleigh.hh and then has some code that we expect to fail compilation (perhaps it can refer to an STL container type without the std:: prefix). And then if it compiles without an error, we can assume that one of the Sleigh headers has a using namespace std; that hasn't been patched out and we can fail the test.

I think I could also just submit a Ghidra PR to fix this as it's pretty uncontroversial that these don't belong in headers.

Any thoughts on this @ekilmer?

@ekilmer
Copy link
Contributor

ekilmer commented Oct 31, 2022

I think I could also just submit a Ghidra PR to fix this as it's pretty uncontroversial that these don't belong in headers.

This would be ideal. Depending on how big the diff of the patch is, we can try maintaining it in this repo for a while until it makes it upstream 🤞.

I opened #140 and set up some branches on our Ghidra fork to maybe make the patching process less painful (https://github.com/trail-of-forks/ghidra/wiki#sleigh).

@mrexodia
Copy link
Contributor Author

It looks like there’s maybe 15 occurrences of using namespace. Unfortunately patching it manually is a lot of work. I gave up after 5 files.

My current idea is to use clangd or maybe write a clang-tidy pass. In this case it’s enough to do it for a few tokens, but in general it’s not so easy…

@ekilmer
Copy link
Contributor

ekilmer commented Nov 5, 2022

@mrexodia can you try #143 to see if it fixes the issues you've been seeing?

@mrexodia
Copy link
Contributor Author

mrexodia commented Nov 5, 2022

Thanks a lot! I'll try it on Monday ❤️

@mrexodia
Copy link
Contributor Author

Unfortunately I didn't have time to try it yet, but I didn't forget about this...

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 a pull request may close this issue.

3 participants