Skip to content

Add DFT planner testing.#653

Merged
shibatch merged 1 commit intomasterfrom
Add_DFT_planner_testing
Mar 10, 2025
Merged

Add DFT planner testing.#653
shibatch merged 1 commit intomasterfrom
Add_DFT_planner_testing

Conversation

@shibatch
Copy link
Copy Markdown
Owner

This PR adds DFT planner testing.

Convert DFT benchmarking utility to C++.
@shibatch shibatch merged commit a9162c4 into master Mar 10, 2025
25 of 26 checks passed
@shibatch shibatch deleted the Add_DFT_planner_testing branch March 10, 2025 10:44
@@ -0,0 +1,29 @@
#if !(defined(__MINGW32__) || defined(__MINGW64__) || defined(_MSC_VER))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not using _WIN32, which is the macro to identify a Windows target, instead of listing different compilers?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Is there a page that systematically explains which macros are defined when using each compiler on Windows? The page you provided is from Microsoft's website, and I don't think it provides reliable information about MinGW.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MinGW does define _WIN32. Tat's the only macro required to be defined by a compiler targeting any Windows system.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The problem with predefined macros is that there is often no proper documentation explaining their intended purpose. In this case, it is difficult to say that the current approach is wrong.

static FILE *OPENTMPFILE() { return tmpfile(); }
static void CLOSETMPFILE(FILE *fp) { fclose(fp); }
#else
#include <Windows.h>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please use lowercase header files, otherwise this breaks compilation on case-sensitive filesystems (e.g. when targeting MinGW on Linux).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

To be honest, it is difficult to support non-standard build methods such as building for MinGW on Linux. Instead, I would suggest that MinGW consider creating a symbolic link from Windows.h to windows.h.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

If the MinGW project adopts our Code of Conduct, I will upgrade MinGW to Tier 1 support.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I'm truly delighted that you seem to be giving serious consideration.
You can understand the content of the CoC in just 30 minutes by watching this video, so please make use of it.

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