birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fixed issue where two Paths could compare equal to each other, but hash differently (#4126)

Browse Source 
* Fixed issue where two Paths could compare equal to each other, but hash
differently

This issue is caused by the Path comparison logic using the path
separator of the left path in a comparison of two paths(left and right)
to determine whether the PathComparison is case-sensitive or not.

The logic has been updated to only perform a non-case-sensitive path
comparison if both paths are using the WindowsPathSeperator of `\`

Also fixed issue with the Hashing algorihtm of the Path class to always
hash the root directory as if it is `/`.
This allows a path of "C:\foo" and "C:/foo" to hash to the equivalent
value.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

* MS Build Tools 14.29 workaround around suppressing warnings using the external header feature

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
This commit is contained in:
lumberyard-employee-dm
2021-09-14 19:21:29 -05:00
committed by GitHub
parent bd8c53550a
commit ce72e32cfc
4 changed files with 148 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/IO/Path/Path.h
+3 -1
View File
@@ -273,7 +273,7 @@ namespace AZ::IO
// If the path input = 'D:bar', then the new PathIterable parts = [D:, 'bar' ]
static constexpr void AppendNormalPathParts(PathIterable& pathIterableResult, const AZ::IO::PathView& path) noexcept;
constexpr int compare_string_view(AZStd::string_view other) const;
constexpr int ComparePathView(const PathView& other) const;
constexpr AZStd::string_view root_name_view() const;
constexpr AZStd::string_view root_directory_view() const;
constexpr AZStd::string_view root_path_raw_view() const;
@@ -480,6 +480,8 @@ namespace AZ::IO
// compare
//! Performs a compare of each of the path parts for equivalence
//! Each part of the path is compare using string comparison
//! If both *this path and the input path uses the WindowsPathSeparator
//! then a non-case sensitive compare is performed
//! Ex: Comparing "test/foo" against "test/fop" returns -1;
//! Path separators of the contained path string aren't compared
//! Ex. Comparing "C:/test\foo" against C:\test/foo" returns 0;
Code/Framework/AzCore/AzCore/IO/Path/Path.inl
+19 -36
View File
@@ -224,15 +224,15 @@ namespace AZ::IO
// compare
constexpr int PathView::Compare(const PathView& other) const noexcept
{
return compare_string_view(other.m_path);
return ComparePathView(other);
}
constexpr int PathView::Compare(AZStd::string_view pathView) const noexcept
{
return compare_string_view(pathView);
return ComparePathView(PathView(pathView, m_preferred_separator));
}
constexpr int PathView::Compare(const value_type* path) const noexcept
{
return compare_string_view(path);
return ComparePathView(PathView(path, m_preferred_separator));
}
constexpr AZStd::fixed_string<MaxPathLength> PathView::FixedMaxPathString() const noexcept
@@ -398,10 +398,10 @@ namespace AZ::IO
return true;
}
constexpr int PathView::compare_string_view(AZStd::string_view pathView) const
constexpr int PathView::ComparePathView(const PathView& other) const
{
auto lhsPathParser = parser::PathParser::CreateBegin(m_path, m_preferred_separator);
auto rhsPathParser = parser::PathParser::CreateBegin(pathView, m_preferred_separator);
auto rhsPathParser = parser::PathParser::CreateBegin(other.m_path, other.m_preferred_separator);
if (int res = CompareRootName(&lhsPathParser, &rhsPathParser); res != 0)
{
@@ -476,6 +476,8 @@ namespace AZ::IO
template <typename PathResultType>
constexpr void PathView::MakeRelativeTo(PathResultType& pathResult, const AZ::IO::PathView& path, const AZ::IO::PathView& base)
{
const bool exactCaseCompare = path.m_preferred_separator == PosixPathSeparator
|| base.m_preferred_separator == PosixPathSeparator;
{
// perform root-name/root-directory mismatch checks
auto pathParser = parser::PathParser::CreateBegin(path.m_path, path.m_preferred_separator);
@@ -487,7 +489,7 @@ namespace AZ::IO
};
if (pathParser.InRootName() && pathParserBase.InRootName())
{
if (int res = Internal::ComparePathSegment(*pathParser, *pathParserBase, pathParser.m_preferred_separator);
if (int res = Internal::ComparePathSegment(*pathParser, *pathParserBase, exactCaseCompare);
res != 0)
{
pathResult.m_path = AZStd::string_view{};
@@ -519,7 +521,7 @@ namespace AZ::IO
auto pathParser = parser::PathParser::CreateBegin(path.m_path, path.m_preferred_separator);
auto pathParserBase = parser::PathParser::CreateBegin(base.m_path, base.m_preferred_separator);
while (pathParser && pathParserBase && pathParser.m_parser_state == pathParserBase.m_parser_state &&
Internal::ComparePathSegment(*pathParser, *pathParserBase, pathParser.m_preferred_separator) == 0)
Internal::ComparePathSegment(*pathParser, *pathParserBase, exactCaseCompare) == 0)
{
++pathParser;
++pathParserBase;
@@ -1080,25 +1082,25 @@ namespace AZ::IO
template <typename StringType>
constexpr int BasicPath<StringType>::Compare(const PathView& other) const noexcept
{
return static_cast<PathView>(*this).compare_string_view(other.m_path);
return static_cast<PathView>(*this).ComparePathView(other);
}
template <typename StringType>
constexpr int BasicPath<StringType>::Compare(const string_type& pathString) const
{
return static_cast<PathView>(*this).compare_string_view(pathString);
return static_cast<PathView>(*this).ComparePathView(PathView(pathString, m_preferred_separator));
}
template <typename StringType>
constexpr int BasicPath<StringType>::Compare(AZStd::string_view pathView) const noexcept
{
return static_cast<PathView>(*this).compare_string_view(pathView);
return static_cast<PathView>(*this).ComparePathView(pathView);
}
template <typename StringType>
constexpr int BasicPath<StringType>::Compare(const value_type* pathString) const noexcept
{
return static_cast<PathView>(*this).compare_string_view(pathString);
return static_cast<PathView>(*this).ComparePathView(pathString);
}
// decomposition
@@ -1330,10 +1332,12 @@ namespace AZ::IO
// PathView::LexicallyRelative is not being used as it returns a FixedMaxPath
// which has a limitation that it requires the relative path to fit within
// an AZ::IO::MaxPathLength buffer
auto ComparePathPart = [pathSeparator = m_preferred_separator](
const bool exactCaseCompare = m_preferred_separator == PosixPathSeparator
|| base.m_preferred_separator == PosixPathSeparator;
auto ComparePathPart = [exactCaseCompare](
const PathIterable::PartKindPair& left, const PathIterable::PartKindPair& right) -> bool
{
return Internal::ComparePathSegment(left.first, right.first, pathSeparator) == 0;
return Internal::ComparePathSegment(left.first, right.first, exactCaseCompare) == 0;
};
const PathIterable thisPathParts = GetNormalPathParts(*this);
@@ -1471,37 +1475,16 @@ namespace AZStd
template <>
struct hash<AZ::IO::PathView>
{
/// Path is using FNV-1a algorithm 64 bit version.
static size_t hash_path(AZStd::string_view pathSegment, const char pathSeparator)
{
size_t hash = 14695981039346656037ULL;
constexpr size_t fnvPrime = 1099511628211ULL;
for (const char first : pathSegment)
{
hash ^= static_cast<size_t>((pathSeparator == AZ::IO::PosixPathSeparator)
? first : tolower(first));
hash *= fnvPrime;
}
return hash;
}
size_t operator()(const AZ::IO::PathView& pathToHash) noexcept
{
auto pathParser = AZ::IO::parser::PathParser::CreateBegin(pathToHash.Native(), pathToHash.m_preferred_separator);
size_t hash_value = 0;
while (pathParser)
{
AZStd::hash_combine(hash_value, hash_path(*pathParser, pathToHash.m_preferred_separator));
++pathParser;
}
return hash_value;
return AZ::IO::parser::HashPath(pathParser);
}
};
template <typename StringType>
struct hash<AZ::IO::BasicPath<StringType>>
{
const size_t operator()(const AZ::IO::BasicPath<StringType>& pathToHash) noexcept
size_t operator()(const AZ::IO::BasicPath<StringType>& pathToHash) noexcept
{
return AZStd::hash<AZ::IO::PathView>{}(pathToHash);
}
Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl
+50 -6
View File
@@ -183,13 +183,12 @@ namespace AZ::IO::Internal
return IsAbsolute(pathView.begin(), pathView.end(), preferredSeparator);
}
// Compares path segments using either Posix or Windows path rules based on the path separator in use
// Posix paths perform a case-sensitive comparison, while Windows paths perform a case-insensitive comparison
static int ComparePathSegment(AZStd::string_view left, AZStd::string_view right, char pathSeparator)
// Compares path segments using either Posix or Windows path rules based on the exactCaseCompare option
static int ComparePathSegment(AZStd::string_view left, AZStd::string_view right, bool exactCaseCompare)
{
const size_t maxCharsToCompare = (AZStd::min)(left.size(), right.size());
int charCompareResult = pathSeparator == PosixPathSeparator
int charCompareResult = exactCaseCompare
? maxCharsToCompare ? strncmp(left.data(), right.data(), maxCharsToCompare) : 0
: maxCharsToCompare ? azstrnicmp(left.data(), right.data(), maxCharsToCompare) : 0;
return charCompareResult == 0
@@ -594,7 +593,10 @@ namespace AZ::IO::parser
{
return pathParser->InRootName() ? **pathParser : "";
};
int res = Internal::ComparePathSegment(GetRootName(lhsPathParser), GetRootName(rhsPathParser), lhsPathParser->m_preferred_separator);
const bool exactCaseCompare = lhsPathParser->m_preferred_separator == PosixPathSeparator
|| rhsPathParser->m_preferred_separator == PosixPathSeparator;
int res = Internal::ComparePathSegment(GetRootName(lhsPathParser), GetRootName(rhsPathParser), exactCaseCompare);
ConsumeRootName(lhsPathParser);
ConsumeRootName(rhsPathParser);
return res;
@@ -621,9 +623,11 @@ namespace AZ::IO::parser
auto& lhsPathParser = *lhsPathParserPtr;
auto& rhsPathParser = *rhsPathParserPtr;
const bool exactCaseCompare = lhsPathParser.m_preferred_separator == PosixPathSeparator
|| rhsPathParser.m_preferred_separator == PosixPathSeparator;
while (lhsPathParser && rhsPathParser)
{
if (int res = Internal::ComparePathSegment(*lhsPathParser, *rhsPathParser, lhsPathParser.m_preferred_separator);
if (int res = Internal::ComparePathSegment(*lhsPathParser, *rhsPathParser, exactCaseCompare);
res != 0)
{
return res;
@@ -646,6 +650,46 @@ namespace AZ::IO::parser
return 0;
}
//path.hash
/// Path is using FNV-1a algorithm 64 bit version.
inline size_t HashSegment(AZStd::string_view pathSegment, bool hashExactPath)
{
size_t hash = 14695981039346656037ULL;
constexpr size_t fnvPrime = 1099511628211ULL;
for (const char first : pathSegment)
{
hash ^= static_cast<size_t>(hashExactPath ? first : tolower(first));
hash *= fnvPrime;
}
return hash;
}
constexpr size_t HashPath(PathParser& pathParser)
{
size_t hash_value = 0;
const bool hashExactPath = pathParser.m_preferred_separator == AZ::IO::PosixPathSeparator;
while (pathParser)
{
switch (pathParser.m_parser_state)
{
case PS_InRootName:
case PS_InFilenames:
AZStd::hash_combine(hash_value, HashSegment(*pathParser, hashExactPath));
break;
case PS_InRootDir:
// Only hash the PosixPathSeparator when a root directory is seen
// This makes the hash consistent for root directories path of C:\ and C:/
AZStd::hash_combine(hash_value, HashSegment("/", hashExactPath));
break;
default:
// The BeforeBegin and AtEnd states contain no segments to hash
break;
}
++pathParser;
}
return hash_value;
}
constexpr int DetermineLexicalElementCount(PathParser pathParser)
{
int count = 0;
Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp
+76
View File
@@ -213,6 +213,82 @@ namespace UnitTest
AZStd::tuple<AZStd::string_view, AZStd::string_view>(R"(foO/Bar)", "foo/bar")
));
struct PathHashCompareParams
{
AZ::IO::PathView m_testPath{};
::testing::Matcher<AZ::IO::PathView> m_compareMatcher;
::testing::Matcher<size_t> m_hashMatcher;
};
class PathHashCompareFixture
: public ScopedAllocatorSetupFixture
, public ::testing::WithParamInterface<PathHashCompareParams>
{};
// Verifies that two paths that compare equal has their hash value compare equal
TEST_P(PathHashCompareFixture, PathsWhichCompareEqual_HashesToSameValue_Succeeds)
{
auto&& [testPath1, compareMatcher, hashMatcher] = GetParam();
// Compare path using parameterized Matcher
EXPECT_THAT(testPath1, compareMatcher);
// Compare hash using parameterized Matcher
const size_t testPath1Hash = AZStd::hash<AZ::IO::PathView>{}(testPath1);
AZ_PUSH_DISABLE_WARNING(4296, "-Wunknown-warning-option")
EXPECT_THAT(testPath1Hash, hashMatcher);
AZ_POP_DISABLE_WARNING
}
INSTANTIATE_TEST_CASE_P(
HashPathCompareValidation,
PathHashCompareFixture,
::testing::Values(
PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::WindowsPathSeparator),
testing::Eq(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)),
testing::Eq(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/foo", AZ::IO::WindowsPathSeparator),
testing::Eq(AZ::IO::PathView(R"(/test/FOO)", AZ::IO::WindowsPathSeparator)),
testing::Eq(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/FOO)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::WindowsPathSeparator),
testing::Eq(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)),
testing::Eq(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::PosixPathSeparator),
testing::Ne(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView(R"(C:\test\foo)", AZ::IO::WindowsPathSeparator),
testing::Ne(AZ::IO::PathView(R"(c:/test/foo)", AZ::IO::PosixPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(c:/test/foo)", AZ::IO::PosixPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator),
testing::Eq(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator)),
testing::Eq(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::PosixPathSeparator),
testing::Gt(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator)),
testing::Eq(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator),
testing::Gt(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::PosixPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::PosixPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/AOO", AZ::IO::PosixPathSeparator),
testing::Lt(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/AOO", AZ::IO::WindowsPathSeparator),
testing::Lt(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::PosixPathSeparator)),
testing::Eq(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::PosixPathSeparator))) },
// Paths with different character values, comparison based on path separator
PathHashCompareParams{ AZ::IO::PathView("/test/BOO", AZ::IO::PosixPathSeparator),
testing::Le(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/BOO", AZ::IO::WindowsPathSeparator),
testing::Ge(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator),
testing::Le(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator))) },
PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::PosixPathSeparator),
testing::Ge(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator)),
testing::Ne(AZStd::hash<AZ::IO::PathView>{}(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator))) }
));
class PathSingleParamFixture
: public ScopedAllocatorSetupFixture
, public ::testing::WithParamInterface<AZStd::tuple<AZStd::string_view>>