Skip to content

Doc comments: use std::unordered_map #11113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
edolstra merged 4 commits into from
Jul 17, 2024
Merged

Doc comments: use std::unordered_map #11113

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c9d62d
Doc comments: use std::unordered_map
roberth Jul 15, 2024
64b4600
Add std::hash<PosIdx>
roberth Jul 16, 2024
d0e9878
Remove unused boost include and split out std-hash.hh
roberth Jul 16, 2024
f5ebaea
Simplify PosIdx::hash()
roberth Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libexpr/eval.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ struct Constant
typedef std::map<std::string, Value *> ValMap;
#endif

typedef std::map<PosIdx, DocComment> DocCommentMap;
typedef std::unordered_map<PosIdx, DocComment> DocCommentMap;

struct Env
{
Expand Down Expand Up @@ -335,7 +335,7 @@ private:
* Associate source positions of certain AST nodes with their preceding doc comment, if they have one.
* Grouped by file.
*/
std::map<SourcePath, DocCommentMap> positionToDocComment;
std::unordered_map<SourcePath, DocCommentMap> positionToDocComment;

LookupPath lookupPath;

Expand Down
2 changes: 1 addition & 1 deletion src/libexpr/parser-state.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ struct LexerState
/**
* @brief Maps some positions to a DocComment, where the comment is relevant to the location.
*/
std::map<PosIdx, DocComment> & positionToDocComment;
std::unordered_map<PosIdx, DocComment> & positionToDocComment;

PosTable & positions;
PosTable::Origin origin;
Expand Down
2 changes: 1 addition & 1 deletion src/libexpr/parser.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@

namespace nix {

typedef std::map<PosIdx, DocComment> DocCommentMap;
typedef std::unordered_map<PosIdx, DocComment> DocCommentMap;

Expr * parseExprFromBuf(
char * text,
Expand Down
23 changes: 23 additions & 0 deletions src/libexpr/pos-idx.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@

#include <cinttypes>

#include "util.hh"

@fzakaria fzakaria Jul 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this included?
Can't seem to tell.

@roberth roberth Jul 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for asking. It was for hash_combine, but that wasn't obvious at all.
I've split that into its own header, partly because "util" is a bad scope for a header, but also for the less subjective argument, from the commit message:

Splitting it out immediately answers questions like [this], without increasing the number of compilation units.

I did consider using boost::hash_combine instead, but it doesn't seem to be quite as capable, accepting only two arguments.


namespace nix {

class PosIdx
{
friend struct LazyPosAcessors;
friend class PosTable;
friend class std::hash<PosIdx>;

private:
uint32_t id;
Expand Down Expand Up @@ -37,8 +40,28 @@ public:
{
return id == other.id;
}

size_t hash() const noexcept
{
size_t h = 854125;
hash_combine(h, id);

@edolstra edolstra Jul 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hash_combine() seems unnecessary? We can just return the hash of id.

@roberth roberth Jul 17, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Java it's common practice to include a random number to represent the distinct type, but I think you're right.
That doesn't really apply here, because we don't have a "top" type like Java's Object that would create a need to avoid collisions when used in e.g. HashMap<String, Object>.
The closest thing we might have is std::variant. I think we can assume that it hashes its index number, which is probably better than salting each type anyway, because iirc it allows multiple distinct occurrences of the same type.

return h;
}
};

inline PosIdx noPos = {};

}

namespace std {

template<>
struct hash<nix::PosIdx>
{
std::size_t operator()(nix::PosIdx pos) const noexcept
{
return pos.hash();
}
};

} // namespace std