Skip to content

Conversation

mat-hek
Copy link
Contributor

@mat-hek mat-hek commented Aug 20, 2025

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later


static bool env_equals(const char *env_var, const char *value)
{
const char *env_value = getenv(env_var);
Copy link
Collaborator

Choose a reason for hiding this comment

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

POSIX says getenv is not thread safe. You need to hold global->env_spinlock.

UNUSED(ctx)
UNUSED(argc)
UNUSED(argv)
if (env_equals("LC_ALL", "UTF-8") || env_equals("LC_CTYPE", "UTF-8") || env_equals("LANG", "UTF-8")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused by this implementation. We could simply always return utf8, couldn't we? The most important part here is the documentation of the function.

@mat-hek mat-hek changed the title Add file:native_name_encoding/0 (#93) Add file:native_name_encoding/0 Aug 21, 2025
Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

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

Let's keep it simple: I think we should always return utf8 and properly document this.
IMHO in this case this doesn't have to be a NIF.

@mat-hek mat-hek force-pushed the mf/file-native-name-encoding branch 2 times, most recently from 91e19cf to 479e989 Compare September 11, 2025 13:40
@mat-hek mat-hek requested a review from bettio September 11, 2025 16:22
@mat-hek
Copy link
Contributor Author

mat-hek commented Sep 11, 2025

@bettio seems like some unrelated tests are timing out/failing on the CI

@bettio
Copy link
Collaborator

bettio commented Sep 19, 2025

@bettio seems like some unrelated tests are timing out/failing on the CI

That's ok, I will re-run CI before merging.

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

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

Only a file rename is required.

% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later
%

-module(test_file_native_name_encoding).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename to test_file so we can consistent with other tests in tests/libs/estdlib/tests.erl.

@bettio bettio mentioned this pull request Sep 19, 2025
@mat-hek mat-hek force-pushed the mf/file-native-name-encoding branch 2 times, most recently from fadedca to 3893e7e Compare September 22, 2025 08:30
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
@mat-hek mat-hek force-pushed the mf/file-native-name-encoding branch from 3893e7e to c63a2ff Compare September 22, 2025 08:36
@mat-hek mat-hek requested a review from bettio September 22, 2025 10:05
@bettio bettio merged commit b94dfea into atomvm:main Sep 22, 2025
167 of 178 checks passed
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.

3 participants