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

clang(windows) getenv deprecation warn fix #900

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexv-ds
Copy link

@alexv-ds alexv-ds commented Jul 18, 2024

getenv in clang(windows) is deprecated

based on spdlog

@ArthurSonzogni
Copy link
Owner

Thanks!

#else
size_t len = 0;
char buf[1024];
bool ok = ::getenv_s(&len, buf, sizeof(buf), field) == 0;
Copy link
Owner

Choose a reason for hiding this comment

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

getenv_s is a C11 function. Is it guaranteed to be defined in C++11?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know.😅

std::string getenv_safe(const char *field) {
#if defined(_MSC_VER)
#if defined(__cplusplus_winrt)
return std::string{}; // not supported under uwp
Copy link
Owner

Choose a reason for hiding this comment

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

Does it means FTXUI won't work under UWP? This sounds problematic.

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I haven't tested this under UWP, but I can assume that UWP applications don't have access to environment variables.

return ok ? buf : std::string{};
#endif
#else // revert to getenv
char *buf = ::getenv(field); // NOLINT(*-mt-unsafe)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe the previous implementation was meant to fix this *-mt-unsafe warning. We probably want to revert it back (e.g. do not store the output in a non temporary object)

bool Contains(const std::string& s, const char* key) {
return s.find(key) != std::string::npos;
}

// https://github.com/gabime/spdlog/blob/885b5473e291833b148eeac3b7ce227e582cd88b/include/spdlog/details/os-inl.h#L566
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any warning on my bots:
https://github.com/ArthurSonzogni/FTXUI/actions/runs/9695462958/job/26755276508

What configs should I add to reproduce?

Are you using the build config the FTXUI project define (e.g. are you using CMake at the end)

Copy link
Author

@alexv-ds alexv-ds Jul 20, 2024

Choose a reason for hiding this comment

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

Oh, I'm sorry. This is not msvc, but clang for windows.

cmake -Bbuild-clang -DCMAKE_CXX_COMPILER=clang++.exe -DCMAKE_C_COMPILER=clang.exe -GNinja
C:/Users/Alex/Desktop/FTXUI/src/ftxui/screen/terminal.cpp:64:37: warning: 'getenv' is deprecated: This function or variable may be unsafe. Consider using _dupenv_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [-Wdeprecated-declarations]
   64 |   std::string COLORTERM = Safe(std::getenv("COLORTERM"));  // NOLINT

Copy link
Author

Choose a reason for hiding this comment

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

@alexv-ds alexv-ds changed the title msvc getenv deprecation warn fix clang(windows) getenv deprecation warn fix Jul 20, 2024
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