Skip to content

Conversation

@NeroBurner
Copy link
Contributor

The variable title is defined as const char*, which means, that
strchr() returns a const char* as well according to
https://www.cplusplus.com/reference/cstring/strchr/

But in the same line the return value is assigned to a non-const
char*, which shouldn't be allowed (error with -pedantic).

To prevent manual memory management just use std::string to copy the
provided title and replace newlines in the copied string.

@JF002
Copy link
Collaborator

JF002 commented Feb 19, 2022

In this case, it's not clear if we use the strchr() function from the C library (char * strchr ( const char *, int ); ) or the const override from the C++ library (const char * strchr ( const char * str, int character );).

According to CLion, strchr() comes from string.h, which is the C version that does not define a const return value.

However, even if we are using the C version, we still modify the content of the string title which beaks the const declaration of the variable, so your concern to totally valid!

I guess the intent of the code was to avoid the copy and the memory overhead of std::string (with potential dynamic allocation).

I'm wondering : is there any other solution that would respect the constness that does not need a std::string temporary variable?

@JF002 JF002 added this to the 1.9.0 milestone Feb 19, 2022
@NeroBurner
Copy link
Contributor Author

Found a solution: lv_label_set_text creates a copy of the string itself. Access and modify that char* instead of the const char* title

Also removed an unused struct in Notifications.h

@NeroBurner NeroBurner force-pushed the notifications_const_char_title_fix branch from 5870cb5 to 9fe2a27 Compare February 19, 2022 22:17
@NeroBurner NeroBurner changed the title Notifications: use std::string instead of strchr because of const char* Notifications: replace newlines in label-copy instead of const char* title directly Feb 19, 2022
@JF002
Copy link
Collaborator

JF002 commented Feb 20, 2022

Did you check that it actually works? The text buffer is edited, but LVGL is not aware of that change. According to the code of lv_label_set_text() you'll probably need to call it again with NULL as parameter, or call lv_label_refr_text() directly.

@NeroBurner
Copy link
Contributor Author

It worked in the simulator, but I'll add the lv_label_refr_text() statement just to be sure

@NeroBurner NeroBurner force-pushed the notifications_const_char_title_fix branch from 5b5cc8c to 15d781b Compare February 20, 2022 11:51
@JF002
Copy link
Collaborator

JF002 commented Feb 20, 2022

Did you check with notification title that contains a \n character?

@NeroBurner
Copy link
Contributor Author

Yes, but only on the Simulator, as it is easier for me to get a notification with a title with newlines there

The shown notification has as input the following title-message-pair:

"category\nUnknown",
"Lorem ipsum\ndolor sit amet,\nconsectetur adipiscing elit,\n",

Unknown

@Riksu9000
Copy link
Contributor

Riksu9000 commented Feb 20, 2022

If the newline is replaced, dolor should be on the first line right?

test

@NeroBurner
Copy link
Contributor Author

The newline is only replaced in the title (the category Unknown part in my example). The newlines in the message are kept as they are (the message is not modified)

NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Feb 20, 2022
InfiniTimeOrg/InfiniTime#976 splits
SystemMonitor.h into a h and cpp file. Add the cpp file to the infinisim
target.
…title

The variable `title` is defined as `const char*`, which means, that
`strchr()` returns a `const char*` as well according to
https://www.cplusplus.com/reference/cstring/strchr/

But in the same line the return value is assigned to a non-const
`char*`, which shouldn't be allowed (error with `-pedantic`).

Because the `lv_label` creates an internal copy of the title sting, just
modify that one instead and replace newline in the copied string.
@NeroBurner NeroBurner force-pushed the notifications_const_char_title_fix branch from 15d781b to 675af69 Compare February 24, 2022 17:45
@NeroBurner
Copy link
Contributor Author

rebased on current develop branch 0e2b27d

NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Feb 24, 2022
InfiniTimeOrg/InfiniTime#976 splits
SystemMonitor.h into a h and cpp file. Add the cpp file to the infinisim
target.
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Feb 26, 2022
InfiniTimeOrg/InfiniTime#976 splits
SystemMonitor.h into a h and cpp file. Add the cpp file to the infinisim
target.
@JF002 JF002 merged commit a29e30c into InfiniTimeOrg:develop Mar 3, 2022
@NeroBurner NeroBurner deleted the notifications_const_char_title_fix branch March 7, 2022 20:38
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