Skip to content

Should we use an enum to encode fields? #75550

New issue

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

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

Already on GitHub? # to your account

Closed
tesuji opened this issue Aug 15, 2020 · 11 comments
Closed

Should we use an enum to encode fields? #75550

tesuji opened this issue Aug 15, 2020 · 11 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tesuji
Copy link
Contributor

tesuji commented Aug 15, 2020

rust/src/librustc_span/lib.rs

Lines 1079 to 1088 in 668a34e

pub struct SourceFile {
/// The name of the file that the source came from. Source that doesn't
/// originate from files has names between angle brackets by convention
/// (e.g., `<anon>`).
pub name: FileName,
/// `true` if the `name` field above has been modified by `--remap-path-prefix`.
pub name_was_remapped: bool,
/// The unmapped path of the file that the source came from.
/// Set to `None` if the `SourceFile` was imported from an external crate.
pub unmapped_path: Option<FileName>,

enum Name {
    /// The name of the file that the source came from. Source that doesn't 
    /// originate from files has names between angle brackets by convention 
    /// (e.g., `<anon>`). 
    Normal(FileName),
    /// FileName modified by `--remap-path-prefix`. 
    Remapped(FileName),
}

pub struct SourceFile { 
    pub name: Name,
    /// The unmapped path of the file that the source came from. 
    /// Set to `None` if the `SourceFile` was imported from an external crate. 
    pub unmapped_path: Option<FileName>, 
    ...
}
@LeSeulArtichaut LeSeulArtichaut added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 16, 2020
@Nicholas-Baron
Copy link
Contributor

I started writing a naive implementation.

  1. Name would have to be pub if the name field in SourceFile is pub
  2. Name has to derive the following at a minimum: Clone, Debug, PartialEq, Eq, Encodable, Decodable, Display
  3. There are some places in the current code where whether a file was remapped is not taken into account. Should there be something like fn unwrap(&self) -> FileName on Name?

PS: I will put a branch up once everything is compiling, unless someone would like to see the code earlier.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 17, 2020

Nice, could you open a draft PR?

@Nicholas-Baron
Copy link
Contributor

@lzutao Draft PR opened.

@Nicholas-Baron
Copy link
Contributor

@rustbot claim

@matklad
Copy link
Member

matklad commented Aug 17, 2020

Super drive by comment, but my gut feeling is that the better fix is to add a type for (name, name_was_remapped, unmpapped_path) triple, rather than go for surface level refactoring of eliminated the bool.

Wild-guessing even further, I'd suggest

pub struct SourceFileName {
    // Note absence of pub
    name: FileName,
    unmapped_name: Option<FileName>
}

impl SourceFileName {
    pub fn name(&self) -> &FileName { &self.name } 
    pub fn unmapped_name(&self) -> &FileName { self.unmpped_name.as_ref().unwrap_or(self.name()) }
}

@petrochenkov
Copy link
Contributor

Yeah, #75616 looks like a regression rather than improvement to me.

@Nicholas-Baron
Copy link
Contributor

@petrochenkov @matklad I have started working on the suggestion. The crate itself and some of its dependents compile.

@Nicholas-Baron
Copy link
Contributor

@matklad Your wild guess seems to be working.

@petrochenkov I hope now #75616 is not a regression.

@Nicholas-Baron
Copy link
Contributor

@rustbot unclaim

I am too distracted to finish the pull request.

@Nicholas-Baron Nicholas-Baron removed their assignment Aug 28, 2020
@rossmacarthur
Copy link
Contributor

rossmacarthur commented Sep 24, 2021

Triage: looks like this was fixed in #83813 :)

@JohnTitor
Copy link
Member

Closing as fixed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants