Skip to content

made config header optional #656

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

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

made config header optional #656

wants to merge 2 commits into from

Conversation

TysonRayJones
Copy link
Member

  • renamed config.h to quest_config.h and made it an optional include which demands no macros are pre-defined, as per the discussions in Fix for quest.h generation #645
  • added a COMPILE_HIP macro merely for book-keeping
  • added TODO for 'installing' in compile.md doc
  • explained quest_config.h in compile.md doc
  • corrected a typo in docs/README.md

- renamed config.h to quest_config.h and made it an optional include which demands no macros are pre-defined, as per the discussions in #645
- added a COMPILE_HIP macro merely for book-keeping
- added TODO for 'installing' in compile.md doc
- explained quest_config.h in compile.md doc
- corrected a typo in docs/README.md
@TysonRayJones
Copy link
Member Author

TysonRayJones commented Jun 26, 2025

@otbrown What do you think of this? Have I accidentally broken installation? 😅

EDIT: woops I've unhelpfully broken everything - I'll re-ping when patched but feel free to comment on the proposed changes generally!

@TysonRayJones
Copy link
Member Author

TysonRayJones commented Jun 26, 2025

Ahh I had colossally brainfarted! Macro FLOAT_PRECISION must always be known by the user's importing code since it informs the qreal and qcomp types and ergo almost every QuEST signature! 😓 Compilation with the non-default FLOAT_PRECISION=2 now will fail.

Perhaps there remains a way to handle it specially so that quest_config.h need not always be included hmm

@otbrown
Copy link
Contributor

otbrown commented Jun 27, 2025

Ah bugger 😅

I was going to suggest making precision.h another CMake config'd file, but then that would permanently break make only building, sooooo, what about:

Using CMake configure_file: precision_config.h.in -> precision_config.h, which just contains #define FLOAT_PRECISION X.

Then in precision.h:

#ifndef FLOAT_PRECISION
#include "precision_config.h"
#endif

CMake users get one extra include file in the includes directory, but otherwise everything looks as it does now. Make users must define FLOAT_PRECISION when compiling either QuEST or onwards codes, or otherwise create precision_config.h?

Edit to add:
Make is really just bash with airs, so one could actually create precision_config.h using echo in the makefile to be fair!

@TysonRayJones
Copy link
Member Author

That configure_file hackery is clever but I worry it's vomit-inducing and prone to cause future confusions.

I really like your idea to just make the otherwise-CMake-made file directly in the makefile or the manual bash script though! That's a much simpler and more elegant hack that's necessary only in the non-primary build method. So "main" CMake users are never exposed to mischief.

I'll probably be slow in getting to this since it's a low-priority and I want to be super cautious I do it in a forward-maintainable way (in case FLOAT_PRECISION turns out not to be the only critical install-time macro)

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