From 7efa1b5745323cc6286ca9041c0641d9b45c846e Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Wed, 2 Sep 2020 10:15:38 -0700 Subject: [PATCH] eden: add CaseSensitive template param to PathMap Summary: This commit adds a compile time option to select between case-sensitive and case-insensitive-but-case-preserving mode for `PathMap`. This replaces the `ifdef _WIN32` preprocessor conditional that was inline in a couple of the methods and allows explicitly testing the behavior in both modes of operation. The unit tests have been expanded and rounded out to catch some inconsistent behavior; insertion wasn't respecting case insensitivity in all ... cases. Hopefully we not relying on that behavior in the windows flavor of the build; let's see what our CI says. Reviewed By: genevievehelsel Differential Revision: D23232629 fbshipit-source-id: 96e752e501d0398ec2bed5879f7c11c7ab6e1d70 --- eden/fs/utils/PathMap.h | 108 ++++++++++++++++++++--------- eden/fs/utils/test/PathMapTest.cpp | 48 +++++++++++++ 2 files changed, 124 insertions(+), 32 deletions(-) diff --git a/eden/fs/utils/PathMap.h b/eden/fs/utils/PathMap.h index d6c4fc77b0..c51be61ffc 100644 --- a/eden/fs/utils/PathMap.h +++ b/eden/fs/utils/PathMap.h @@ -7,6 +7,7 @@ #pragma once #include +#include #include #include #include @@ -16,6 +17,8 @@ namespace facebook { namespace eden { +constexpr bool kPathMapDefaultCaseSensitive = !folly::kIsWindows; + /** An associative container that maps from one of our path types to an * arbitrary value type. * @@ -30,7 +33,10 @@ namespace eden { * - Since insert and erase operations move the vector contents around, * those operations invalidate iterators. */ -template +template < + typename Value, + typename Key = PathComponent, + bool CaseSensitive = kPathMapDefaultCaseSensitive> class PathMap : private folly::fbvector> { using Pair = std::pair; using Vector = folly::fbvector; @@ -164,18 +170,17 @@ class PathMap : private folly::fbvector> { // Found it return iter; } -#ifdef _WIN32 - // On Windows we need to do a case insensitive lookup for the file and - // directory names. For performance, we will do a case sensitive search - // first which should cover most of the cases and if not found then do a - // case sensitive search. - for (iter = begin(); iter != end(); ++iter) { - if (key.stringPiece().equals( - iter->first.stringPiece(), folly::AsciiCaseInsensitive())) { - return iter; + if (!CaseSensitive) { + // When !CaseSensitive, for performance, we will do a case sensitive + // search first which should cover most of the cases and if not found then + // do a case insensitive search. + for (iter = begin(); iter != end(); ++iter) { + if (key.stringPiece().equals( + iter->first.stringPiece(), folly::AsciiCaseInsensitive())) { + return iter; + } } } -#endif return end(); } @@ -188,18 +193,14 @@ class PathMap : private folly::fbvector> { // Found it return iter; } -#ifdef _WIN32 - // On Windows we need to do a case insensitive lookup for the file and - // directory names. For performance, we will do a case sensitive search - // first which should cover most of the cases and if not found then do a - // case sensitive search. - for (iter = begin(); iter != end(); ++iter) { - if (key.stringPiece().equals( - iter->first.stringPiece(), folly::AsciiCaseInsensitive())) { - return iter; + if (!CaseSensitive) { + for (iter = begin(); iter != end(); ++iter) { + if (key.stringPiece().equals( + iter->first.stringPiece(), folly::AsciiCaseInsensitive())) { + return iter; + } } } -#endif return end(); } @@ -209,10 +210,24 @@ class PathMap : private folly::fbvector> { * a boolean that is true if an insert took place. */ std::pair insert(const value_type& val) { auto iter = lower_bound(val.first); - if (iter == end() || compare_(val.first, iter->first)) { - return std::make_pair(Vector::insert(iter, val), true); + + if (iter != end() && !compare_(val.first, iter->first)) { + // Found it; leave it alone + return std::make_pair(iter, false); } - return std::make_pair(iter, false); + + if (!CaseSensitive) { + for (auto insens = begin(); insens != end(); ++insens) { + if (val.first.stringPiece().equals( + insens->first.stringPiece(), folly::AsciiCaseInsensitive())) { + // Found it; leave it alone + return std::make_pair(insens, false); + } + } + } + + // Otherwise, iter is the insertion point + return std::make_pair(Vector::insert(iter, val), true); } /** Emplace a new key-value pair by constructing it in-place. @@ -224,22 +239,51 @@ class PathMap : private folly::fbvector> { template std::pair emplace(Piece key, Args&&... args) { auto iter = lower_bound(key); - if (iter == end() || compare_(key, iter->first)) { - iter = Vector::emplace( - iter, std::make_pair(Key(key), Value(std::forward(args)...))); - return std::make_pair(iter, true); + + if (iter != end() && !compare_(key, iter->first)) { + // Found it; leave it alone + return std::make_pair(iter, false); } - return std::make_pair(iter, false); + + if (!CaseSensitive) { + for (auto insens = begin(); insens != end(); ++insens) { + if (key.stringPiece().equals( + insens->first.stringPiece(), folly::AsciiCaseInsensitive())) { + // Found it; leave it alone + return std::make_pair(insens, false); + } + } + } + + // Otherwise, iter is the insertion point + iter = Vector::emplace( + iter, std::make_pair(Key(key), Value(std::forward(args)...))); + return std::make_pair(iter, true); } /** Returns a reference to the map position for key, creating it needed. * If the key is already present, no additional allocations are performed. */ mapped_type& operator[](Piece key) { auto iter = lower_bound(key); - if (iter == end() || compare_(key, iter->first)) { - // Not yet present, make a new one - iter = Vector::insert(iter, std::make_pair(Key(key), mapped_type())); + + if (iter != end() && !compare_(key, iter->first)) { + // Found it + return iter->second; } + + if (!CaseSensitive) { + // Case insensitive lookup + for (auto insens = begin(); insens != end(); ++insens) { + if (key.stringPiece().equals( + insens->first.stringPiece(), folly::AsciiCaseInsensitive())) { + // Found it + return insens->second; + } + } + } + + // Not yet present, make a new one at the insertion point + iter = Vector::insert(iter, std::make_pair(Key(key), mapped_type())); return iter->second; } diff --git a/eden/fs/utils/test/PathMapTest.cpp b/eden/fs/utils/test/PathMapTest.cpp index 7e8c87838b..b8d860acaf 100644 --- a/eden/fs/utils/test/PathMapTest.cpp +++ b/eden/fs/utils/test/PathMapTest.cpp @@ -14,6 +14,54 @@ using facebook::eden::PathComponentPiece; using facebook::eden::PathMap; using namespace facebook::eden::path_literals; +TEST(PathMap, caseSensitive) { + // Explicitly a case sensitive map, regardless of the host OS + PathMap map; + + map.insert(std::make_pair(PathComponent("foo"), true)); + EXPECT_TRUE(map.at("foo"_pc)); + EXPECT_EQ(map.find("Foo"_pc), map.end()); + + EXPECT_TRUE(map.insert(std::make_pair(PathComponent("FOO"), false)).second); + EXPECT_EQ(map.size(), 2); + EXPECT_TRUE(map.at("foo"_pc)); + EXPECT_FALSE(map.at("FOO"_pc)); + EXPECT_EQ(map.erase("FOO"_pc), 1); + EXPECT_EQ(map.size(), 1); + + map["FOO"_pc] = true; + map["Foo"_pc] = false; + EXPECT_EQ(map.size(), 3); +} + +TEST(PathMap, caseInSensitive) { + // Explicitly a case IN-sensitive map, regardless of the host OS + PathMap map; + + map.insert(std::make_pair(PathComponent("foo"), true)); + EXPECT_TRUE(map.at("foo"_pc)); + EXPECT_TRUE(map.at("Foo"_pc)); + + EXPECT_FALSE(map.insert(std::make_pair(PathComponent("FOO"), false)).second); + EXPECT_FALSE(map.emplace(PathComponent("FOO"), false).second); + EXPECT_EQ(map.size(), 1); + EXPECT_TRUE(map.at("foo"_pc)); + EXPECT_TRUE(map.at("FOO"_pc)); + + EXPECT_EQ(map.erase("FOO"_pc), 1); + EXPECT_EQ(map.size(), 0); + + // Case insensitive referencing + map["FOO"_pc] = true; + map["Foo"_pc] = false; + // Only one FOO entry + EXPECT_EQ(map.size(), 1); + // It shows as false + EXPECT_EQ(map["FOO"_pc], false); + // The assignment above didn't change the case of the key! + EXPECT_EQ(map.begin()->first, "FOO"_pc); +} + TEST(PathMap, insert) { PathMap map;