allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

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

Open KangarooKoala opened this issue 1 year ago • 4 comments

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.

KangarooKoala avatar Dec 30 '23 23:12 KangarooKoala

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.

calcmogul avatar Dec 31 '23 00:12 calcmogul

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).

Starlight220 avatar Apr 27 '24 19:04 Starlight220

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 avatar Apr 27 '24 21:04 KangarooKoala

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 avatar Apr 29 '24 03:04 KangarooKoala

(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)

KangarooKoala avatar May 30 '24 00:05 KangarooKoala

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.

KangarooKoala avatar Jun 01 '24 05:06 KangarooKoala