Page MenuHomePhabricator

[llvm][Support] Delete unused is_absolute_gnu helper
Needs ReviewPublic

Authored by kadircet on Sep 29 2020, 10:19 AM.

Details

Reviewers
tinti

Diff Detail

Event Timeline

kadircet created this revision.Sep 29 2020, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2020, 10:19 AM
kadircet requested review of this revision.Sep 29 2020, 10:19 AM
certl added a subscriber: certl.Sep 30 2020, 3:16 AM

The other path manipulation functions have similar quirks for Windows paths (Unicode drives / NTFS alternate streams). I'm not particularly concerned, but for correctness sake I would also recommend for those to be removed / refactored / replaced with something standard such as those from C++17.

find_first_component - assumes all drives are single byte alpha characters (see here for how the git project handles this):

if (real_style(style) == Style::windows) {
  // C:
  if (path.size() >= 2 &&
      std::isalpha(static_cast<unsigned char>(path[0])) && path[1] == ':')
    return path.substr(0, 2);
}

root_dir_start - also assumes single byte drive:

// case "c:/"
if (real_style(style) == Style::windows) {
  if (str.size() > 2 && str[1] == ':' && is_separator(str[2], style))
    return 2;
}

filename_pos - doesn't anticipate NTFS alternate streams (eg: I believe C:\xxx\yyy::$INDEX_ALLOCATION\ would return the filename as $INDEX_ALLOCATION), as well as assuming single byte drives:

if (real_style(style) == Style::windows) {
  if (pos == StringRef::npos)
    pos = str.find_last_of(':', str.size() - 2);
}
kadircet added a subscriber: tinti.

Hi @tinti this code seems to be recently introduced without any usage, and as pointed out by @certl it doesn't consider multi-byte drive letters.

Are you planning to add any usages to the helper? If so, please consider Christopher's points about potential problems.
If you are not planning to add any usages, I beleive it would be best to delete the helper to reduce confusion.

WDYT?

tinti added a comment.Oct 7 2020, 4:26 PM

@

Hi @tinti this code seems to be recently introduced without any usage, and as pointed out by @certl it doesn't consider multi-byte drive letters.

Hi @certl and @kadircet.

It intentionally does not consider multi-byte drive letters because it mimics GNU's objdump (which does not either).

Are you planning to add any usages to the helper? If so, please consider Christopher's points about potential problems.
If you are not planning to add any usages, I beleive it would be best to delete the helper to reduce confusion.

Yes I am. I am about to submit another patch for llvm-objdump.

WDYT?

We considered using LLVM's helpers but would be too much work. It is simpler to do this way.

Please take a look at: https://reviews.llvm.org/D87667