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

[wpiutil C++] Add remove_prefix() and remove_suffix() #6118

Merged
merged 15 commits into from
Jun 5, 2024

Conversation

KangarooKoala
Copy link
Contributor

Notes:

  • Some spots that used drop_front(str, prefix.size()) didn't previously check that the value began with the prefix, so this might change the behavior of those spots. (There's probably other ways that the property is guaranteed, like with the listener prefixes, but I didn't completely verify that)
  • There's also a spot where I make use of the behavior that remove_suffix() doesn't change a string not ending with the suffix. I'm not sure if the code is the clearest, though- I may revert this. (Please give feedback on this)
  • In NetworkTablesProvider, I made my best guess on where the 6 came from based on the rest of the function, but I'd like to make sure someone double-checks that it makes sense.
  • I'll see if CI passes, but when running wpiformat locally, it'd just say "Warning: generated file <...>/StringExtras.h modified" instead of actually formatting it. I manually formatted the doc comment, but the function declaration probably also needs formatting.

@calcmogul
Copy link
Member

calcmogul commented Dec 31, 2023

StringExtras.h is considered a generated file because it used to be updated by the update_llvm.py script. It's now in limbo because it's an LLVM file we manually modify. Whether we format it depends on whether we plan on updating pieces from upstream again. Idk.

PeterJohnson
PeterJohnson previously approved these changes Feb 5, 2024
@Starlight220
Copy link
Member

Most of the usages are of the following idiom:

if str.startswith(literal):
  str = remove_prefix(str, literal)

Two questions:

  • do we want to extract the literal to a variable? Could prevent mistakes caused by the two literals not being identical.
  • do we need the explicit call-site check? It's checked in the implementation as well, which makes the double check redundant (I guess it might be optimized away, but still).

@KangarooKoala
Copy link
Contributor Author

do we want to extract the literal to a variable? Could prevent mistakes caused by the two literals not being identical.

We might, but I personally am fine with relying on the review process checking the literals are identical (especially compared to checking the length matches that of the literal), which generally isn't that bad since usually they're within a few lines of each other. I also think it'd be annoying to have to pick a spot to declare the variable with the value of the literal, but I get the concerns over keeping both spots in sync with each other.

do we need the explicit call-site check?

The ones with an explicit call-site check usually do other logic, so you need the explicit check anyways. We could maybe add a parameter to prevent remove_prefix(), but I'm not sure if that's worth it.

@KangarooKoala
Copy link
Contributor Author

I'm trying a change (d0de6af) to fix the MacOS build error, but it probably should get moved into a separate PR once it's verified to work.

@KangarooKoala
Copy link
Contributor Author

KangarooKoala commented May 30, 2024

(Note for myself so I don't forget)
Peter:

I’m still a bit on the fence re: the remove_prefix PR. I don’t really like the repeated string, and for a lot of use cases, it feels like the function signature isn’t quite right. It feels like something like std::optional<std::string_view> would be a better return value (returning nullopt if the prefix isn’t present) so you could put it directly in an if statement with no repetition, and for cases you want to just remove a prefix if it is present, you could do .or_else(original_string)

Local patch to fix compilation that should not be on main
Also fix a spot that erronerously translated drop_back to remove_prefix
@KangarooKoala
Copy link
Contributor Author

I'm not the happiest with a few of the translations- I think Peter had a really good vision for the most common use-case, but some of the odder occurrences were difficult to do cleanly. (Specifically, the isArray check in glass/src/libnt/native/cpp/NetworkTables.cpp, and spots with multiple checks in the same condition (with else-ifs that come after that prevent refactoring into separate if statements).)
Also, the PR will change some spots to throw std::bad_optional_access if the value does not actually start with the expected prefix (or end with the expected suffix). I don't think it'd be problematic (We'd have to time before next season to see if there were any spots that actually didn't meet that condition and fix them), but it's still worth noting.

glass/src/libnt/native/cpp/NTField2D.cpp Outdated Show resolved Hide resolved
glass/src/libnt/native/cpp/NTField2D.cpp Outdated Show resolved Hide resolved
glass/src/libnt/native/cpp/NTMechanism2D.cpp Outdated Show resolved Hide resolved
glass/src/libnt/native/cpp/NetworkTables.cpp Outdated Show resolved Hide resolved
glass/src/libnt/native/cpp/NetworkTables.cpp Outdated Show resolved Hide resolved
glass/src/libnt/native/cpp/NetworkTablesProvider.cpp Outdated Show resolved Hide resolved
ntcore/src/main/native/cpp/LocalStorage.cpp Outdated Show resolved Hide resolved
Use value_or instead of value (to avoid throwing)
Dereference optionals
Undo accidental change to NetworkTablesModel::Update() behavior schemas of arrays
@PeterJohnson PeterJohnson merged commit d6b66bf into wpilibsuite:main Jun 5, 2024
25 checks passed
@KangarooKoala KangarooKoala deleted the remove-prefix-suffix branch June 5, 2024 04:02
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.

4 participants