Skip to content

Commit

Permalink
Make source_location use string_view instead of const string&
Browse files Browse the repository at this point in the history
Summary:
`const std::string&` requires the caller to copy if all they have is `std::string_view`. Let's modernize the code here.

Heterogenous lookups are not supported by `std::unordered_map` until C++20 so I've had to switch to `std::map`. This shouldn't be a problem in practice.

Change is no-op.

Reviewed By: iahs

Differential Revision: D69197835

fbshipit-source-id: 83b0849a5bada97f9c41b13845a5567aeb2f2694
  • Loading branch information
praihan authored and facebook-github-bot committed Feb 6, 2025
1 parent 75743cb commit c123e30
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 30 deletions.
36 changes: 18 additions & 18 deletions third-party/thrift/src/thrift/compiler/source_location.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@

#include <fmt/core.h>

#include <assert.h>
#include <errno.h>
#include <string.h>
#include <algorithm>
#include <cassert>
#include <cerrno>
#include <cstring>
#include <filesystem>
#include <stdexcept>
#include <string>
Expand Down Expand Up @@ -84,24 +84,24 @@ std::vector<uint_least32_t> get_line_offsets(std::string_view sv) {
} // namespace

source source_manager::add_source(
const std::string& file_name, std::vector<char> text) {
std::string_view file_name, std::vector<char> text) {
assert(text.back() == '\0');
std::string_view sv(text.data(), text.size());
sources_.push_back(
source_info{file_name, std::move(text), get_line_offsets(sv)});
sources_.push_back(source_info{
std::string(file_name), std::move(text), get_line_offsets(sv)});
return {/* .start = */
source_location(/* source_id= */ sources_.size(), /** offset= */ 0),
/* .text = */ sv};
}

std::string source_manager::get_file_path(const std::string& file_name) const {
std::string source_manager::get_file_path(std::string_view file_name) const {
if (file_source_map_.find(file_name) != file_source_map_.end()) {
return file_name;
return std::string(file_name);
}
return std::filesystem::absolute(file_name).string();
}

std::optional<source> source_manager::get_file(const std::string& file_name) {
std::optional<source> source_manager::get_file(std::string_view file_name) {
if (auto source = file_source_map_.find(file_name);
source != file_source_map_.end()) {
return source->second;
Expand Down Expand Up @@ -156,11 +156,11 @@ std::optional<source> source_manager::get_file(const std::string& file_name) {
}

source source_manager::add_virtual_file(
const std::string& file_name, const std::string& src) {
std::string_view file_name, std::string_view src) {
if (file_source_map_.find(file_name) != file_source_map_.end()) {
throw std::runtime_error(std::string("file already added: ") + file_name);
throw std::runtime_error(fmt::format("file already added: {}", file_name));
}
const char* start = src.c_str();
const char* start = src.data();
auto source =
add_source(file_name, std::vector<char>(start, start + src.size() + 1));
file_source_map_.emplace(file_name, source);
Expand Down Expand Up @@ -202,21 +202,21 @@ resolved_location::resolved_location(
}

source_manager::path_or_error source_manager::find_include_file(
const std::string& filename,
const std::string& parent_path,
std::string_view filename,
std::string_view parent_path,
const std::vector<std::string>& search_paths) {
if (auto itr = found_includes_.find(filename); itr != found_includes_.end()) {
return source_manager::path_or_error{std::in_place_index<0>, itr->second};
}

auto found = [&](std::string path) {
found_includes_[filename] = path;
found_includes_[std::string(filename)] = path;
return source_manager::path_or_error{
std::in_place_index<0>, std::move(path)};
};

if (file_source_map_.find(filename) != file_source_map_.end()) {
return found(filename);
return found(std::string(filename));
}

// Absolute path? Just try that.
Expand All @@ -236,7 +236,7 @@ source_manager::path_or_error source_manager::find_include_file(
// new search path with current dir global
std::vector<std::string> sp = search_paths;
auto itr = found_includes_.find(parent_path);
const std::string& resolved_parent_path =
std::string_view resolved_parent_path =
itr != found_includes_.end() ? itr->second : parent_path;
auto dir = std::filesystem::path(resolved_parent_path).parent_path().string();
dir = dir.empty() ? "." : dir;
Expand Down Expand Up @@ -271,7 +271,7 @@ source_manager::path_or_error source_manager::find_include_file(
}

std::optional<std::string> source_manager::found_include_file(
const std::string& filename) const {
std::string_view filename) const {
if (auto itr = found_includes_.find(filename); itr != found_includes_.end()) {
return itr->second;
}
Expand Down
24 changes: 12 additions & 12 deletions third-party/thrift/src/thrift/compiler/source_location.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@

#pragma once

#include <stdint.h>

#include <cstdint>
#include <deque>
#include <functional>
#include <map>
#include <optional>
#include <string>
#include <unordered_map>
#include <string_view>
#include <variant>
#include <vector>

Expand Down Expand Up @@ -115,10 +115,10 @@ class source_manager {
// sources_ grows.
std::deque<source_info> sources_;

std::unordered_map<std::string, source> file_source_map_;
std::map<std::string, source, std::less<>> file_source_map_;

// Maps from filepaths present in the AST to filepaths on disk.
std::unordered_map<std::string, std::string> found_includes_;
std::map<std::string, std::string, std::less<>> found_includes_;

const source_info* get_source(uint_least32_t source_id) const {
return source_id > 0 && source_id <= sources_.size()
Expand All @@ -128,20 +128,20 @@ class source_manager {

friend class resolved_location;

source add_source(const std::string& file_name, std::vector<char> text);
source add_source(std::string_view file_name, std::vector<char> text);

public:
// Loads a file and returns a source object representing its content.
// The file can be a real file or a virtual one previously registered with
// add_virtual_file.
// Returns an empty optional if opening or reading the file fails.
// Makes use of the result of previous calls to find_include_file.
std::optional<source> get_file(const std::string& file_name);
std::optional<source> get_file(std::string_view file_name);

std::string get_file_path(const std::string& file_name) const;
std::string get_file_path(std::string_view file_name) const;

// Adds a virtual file with the specified name and content.
source add_virtual_file(const std::string& file_name, const std::string& src);
source add_virtual_file(std::string_view file_name, std::string_view src);

// Returns the start location of a source containing the specified location.
// It is a member function in case we add clang-like compression of locations.
Expand Down Expand Up @@ -171,12 +171,12 @@ class source_manager {
// The resolved path is made available to get_file.
using path_or_error = std::variant<std::string, std::string>;
path_or_error find_include_file(
const std::string& filename,
const std::string& program_path,
std::string_view filename,
std::string_view program_path,
const std::vector<std::string>& search_paths);
// Queries for a file previously found by find_include_file.
std::optional<std::string> found_include_file(
const std::string& filename) const;
std::string_view filename) const;
};

} // namespace apache::thrift::compiler

0 comments on commit c123e30

Please sign in to comment.