Skip to content
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.

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

Already on GitHub? Sign in to your account

improve documentation for DsymbolTable #12570

Merged
merged 1 commit into from May 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 32 additions & 12 deletions src/dmd/dsymbol.d
Expand Up @@ -2186,41 +2186,61 @@ extern (C++) final class DsymbolTable : RootObject
{
AssocArray!(Identifier, Dsymbol) tab;

// Look up Identifier. Return Dsymbol if found, NULL if not.
/***************************
* Look up Identifier in symbol table
* Params:
* ident = identifer to look up
* Returns:
* Dsymbol if found, null if not
*/
Dsymbol lookup(const Identifier ident)
{
//printf("DsymbolTable::lookup(%s)\n", ident.toChars());
return tab[ident];
}

// Look for Dsymbol in table. If there, return it. If not, insert s and return that.
Dsymbol update(Dsymbol s)
/**********
* Replace existing symbol in symbol table with `s`.
* If it's not there, add it.
* Params:
* s = replacement symbol with same identifier
*/
void update(Dsymbol s)
{
const ident = s.ident;
Dsymbol* ps = tab.getLvalue(ident);
*ps = s;
return s;
*tab.getLvalue(s.ident) = s;
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this, it's not an improvement to the documentation.

(Also forgot to update dsymbol.h...)

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is tagged "refactoring".

Updated dsymbol.h.

Copy link
Contributor

Choose a reason for hiding this comment

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

A refactoring should not change the behavior... Have you checked that this change doesn't affect LDC?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't change the behavior - dmd never calls it. As for gdc/ldc, if they're using the return value, it'll give an error, not silently corrupt anything. The return value makes no sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

It occurs to me you and I have different interpretations of what "refactoring" means. To me, it means not changing the behavior of the program. To you (correct me if I'm wrong) it means not changing the API of any functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Refactorings should retain the behavior of a component, it's more nuanced than the whole program but doesn't prohibit changes to function signatures.

The problem here is a change to the public API which can break code depending on "DMD as a library" - without providing any real benefit.

But I guess that is acceptable given that the potential breakage isn't silent and can easily be fixed.

}

// Insert Dsymbol in table. Return NULL if already there.
/**************************
* Insert Dsymbol in table.
* Params:
* s = symbol to add
* Returns:
* null if already in table, `s` if inserted
*/
Dsymbol insert(Dsymbol s)
{
//printf("DsymbolTable::insert(this = %p, '%s')\n", this, s.ident.toChars());
return insert(s.ident, s);
}

// when ident and s are not the same
/**************************
* Insert Dsymbol in table.
* Params:
* ident = identifier to serve as index
* s = symbol to add
* Returns:
* null if already in table, `s` if inserted
*/
Dsymbol insert(const Identifier ident, Dsymbol s)
{
//printf("DsymbolTable::insert()\n");
//printf("DsymbolTable.insert(this = %p, '%s')\n", this, s.ident.toChars());
Dsymbol* ps = tab.getLvalue(ident);
if (*ps)
return null; // already in table
*ps = s;
return s;
}

/*****
/*****************
* Returns:
* number of symbols in symbol table
*/
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/dsymbol.h
Expand Up @@ -387,7 +387,7 @@ class DsymbolTable : public RootObject
Dsymbol *lookup(Identifier const * const ident);

// Look for Dsymbol in table. If there, return it. If not, insert s and return that.
Dsymbol *update(Dsymbol *s);
void update(Dsymbol *s);

// Insert Dsymbol in table. Return NULL if already there.
Dsymbol *insert(Dsymbol *s);
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/frontend.h
Expand Up @@ -3115,7 +3115,7 @@ class DsymbolTable final : public RootObject
public:
AssocArray<Identifier*, Dsymbol* > tab;
Dsymbol* lookup(const Identifier* const ident);
Dsymbol* update(Dsymbol* s);
void update(Dsymbol* s);
Dsymbol* insert(Dsymbol* s);
Dsymbol* insert(const Identifier* const ident, Dsymbol* s);
size_t length() const;
Expand Down