Why PathId holds a reference to its owner SymbolTable?

[hs-apotell]

For reference, this was discussed briefly in few various contexts -

• Food for thought: SymbolId can carry a pointer to the owning SymbolTable #3054
• IO Abstraction: Introducing PathId & FileSystem (Phase 1) #3219
• Issue #3221, #3219: Follow up and fixing warnings #3223 (comment)

Proposed: SymbolTable can hold reference to owning SymbolTable.

Pros:

• Less error prone
• Code is elegant (one argument vs. two arguments)
• Avoid holding symbols in multiple SymbolTables (duplication)
• Can potentially save on memory in SymbolTable as the index lookup won't be required

Cons:

• Id resolving to its value isn't very intuitive (rename?)
• Large code change

The proposed strategy was implemented in PathId. For SymbolId, it was merely a way to ease its use but for PathId, in some context, it is a necessity. May be the term PathId isn't appropriate but the need for it is justified.

PathId is intended to always hold an absolute path i.e. rooted path and FileSystem is intended to be extended in client application to add features that are either application specific or just doesn't belong in Surelog for any number of reasons. In some cases, it comes down to licensing issues as well because of third party libraries that might be involved in extending the functionality. All transactions between Surelog and extended FileSystem are using PathId. The API is much cleaner if that doesn't involve having to deal with SymbolTable as well.

It indicates that the user of the FileId does not know which SymbolTable it belongs to.

Yes, the client application doesn't and it doesn't have to. FileSystem is a singleton and it is intended to be accessed by all threads in the system. The API doesn't know the context in which a particular API is being called. The caller does and yes, the SymbolTable can be passed as an additional argument to the API. That's an additional argument to every API call in the FileSystem.

Why do we even expect it to be in different SymbolTables ? It sounds like we want one SymbolTable in the filesystem implementation, where only the filesystem hands out new IDs.

SymbolTable themselves aren't thread-safe. I did try to introduce a version to make it thread-safe but it was deemed inappropriate within the overall design (each preprocessor and compiler has its own SymbolTable). FileSystem itself doesn't have its own SymbolTable. But potentially could but this implementation will have to be thread-safe because FileSystem itself is a singleton, used across any number of threads. In the current implementation, only a handful of APIs are deemed necessary to be thread-safe. With FileSystem owning its own SymbolTable, all APIs will need to be thread-safe.

Plus, FileSystem is intended to be extended in client application. Do we really want to force clients to implement a thread-safe FileSystem API? I hold my reservations on that.

There might be a better design for PathId or may be PathId itself shouldn't be called PathId.

I am open to suggestions/discussions and making the change itself.

P.S. Please take a moment to review the attached FileSystem implementation. This is complete implementation and should help decide the direction of where this discussion goes.

FileSystem.h.txt
FileSystem.cpp.txt

[hs-apotell]

@hzeller @alaindargelas I am about to create additional PRs that are on top of the one that was submitted last week. @hzeller raised some concerns/questions about my design decision to have a SymbolTable reference in PathId. The new PR are compiled based on this design and so would like to confirm whether, if any, changes are needed to the existing design before I make more changes. It will be easier to make those changes now before I add more code on top of it.

Please take a moment to review the design decision and raise any concerns/questions that you might have. Let the conversation flow.