Skip to content

Fix warning in FASM #2412

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

Closed
vaughnbetz opened this issue Oct 6, 2023 · 4 comments
Closed

Fix warning in FASM #2412

vaughnbetz opened this issue Oct 6, 2023 · 4 comments

Comments

@vaughnbetz
Copy link
Contributor

Recent CI shows this warning in fasm:

/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/utils/fasm/src/fasm.cpp:572:24: warning: loop variable ‘param’ creates a copy from type ‘const std::__cxx11::basic_string’ [-Wrange-loop-construct]
572 | for(const auto param : vtr::split(fasm_params_str, "\n")) {
| ^~~~~
/home/runner/work/vtr-verilog-to-routing/vtr-verilog-to-routing/utils/fasm/src/fasm.cpp:572:24: note: use reference type to prevent copying
572 | for(const auto param : vtr::split(fasm_params_str, "\n")) {
| ^~~~~
| &

@duck2 : can you fix this? I think it just needs a ref (&).

@hzeller
Copy link
Contributor

hzeller commented Oct 6, 2023

In general, not only using string references, but also using more std::string_view would help avoid a lot of allocating new strings. I see that there are a lot of functions that take a std::string as value parameter and return std::vector<std::string> (e.g. split_fasm_entry()).

(And for fast fasm parsing, I'd like to plug this thing I did a while back https://github.com/hzeller/simple-fasm - feel free to just copy into the project).

@vaughnbetz
Copy link
Contributor Author

@hzeller: if that new fasm works better I'm happy to see it integrated, but I think you'd have to drive it since I don't have any fasm experts on my team.

@AlexandreSinger
Copy link
Contributor

This warning has been resolved in PR #2535

@vaughnbetz should we close this issue?

@vaughnbetz
Copy link
Contributor Author

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

No branches or pull requests

3 participants