Improvements to `proc_macro::Span` API #43604

abonander · 2017-08-02T01:59:52Z

Motivation: https://internals.rust-lang.org/t/better-panic-location-reporting-for-unwrap-and-friends/5042/12?u=logician

TODO:

Bikeshedding/complete API
Implement tests/verify return values

cc @jseyfried @nrc

rust-highfive · 2017-08-02T02:00:04Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

abonander · 2017-08-02T02:00:18Z

r? @jseyfried

abonander · 2017-08-02T02:02:04Z

src/libproc_macro/lib.rs

+    pub fn glue(&self, other: Span) -> Option<Span> {
+        if self == other { return None }
+
+        if self.0.lo != other.0.hi || self.0.hi != other.0.lo { return None }


Missing case: overlapping but no equivalent positions

abonander · 2017-08-02T02:03:16Z

Should Span::glue() just wrap syntax_pos::Span::to()?

est31 · 2017-08-02T04:10:06Z

@abonander you might be interested in this RFC: rust-lang/rfcs#2091
cc @kennytm

abonander · 2017-08-02T04:41:36Z

Yeah, I read it. This is for more than just rewrite_unwraps, though; it can be used for more detailed error handling by spitting out a source file/line for errors.

abonander · 2017-08-04T21:54:34Z

src/libproc_macro/lib.rs

+
+    /// The original source file into which this span points.
+    #[unstable(feature = "proc_macro", issue = "38356")]
+    pub fn file(&self) -> SourceFile {


Should this return Option<SourceFile> instead, for when the input to the current macro is the result of another macro and thus will not have a source file?

Wouldn't the source file be the file in which the other macro was defined then?
I still think this should be Option<SourceFile>, at least for impl FromStr for TokenStream (i.e. DUMMY_SP).

Hmm, I need to check the values for the other methods that DUMMY_SP might return to make sure they're not nonsense or else pointing into something unrelated to the current macro expansion.

abonander · 2017-08-12T08:46:36Z

src/test/compile-fail-fulldeps/proc-macro/span-api-tests.rs

+use span_api_tests::{reemit, assert_no_source_file};
+
+reemit! {
+    assert_no_source_file! { "Hello, world!" }


@jseyfried The "Hello, world!" token here gives its source file as span-api-tests.rs even though it was reparsed from a string by reemit! { }. Is this the correct behavior? If so, we can't rely on the source file being "<anon>" to determine if it was emitted from a macro or not. I think the output from external crates' macros is marked as "<anon>" though.

Re-parsing from a string creates tokens with the macro invocation span and then applies a mark to indicate that they are macro-expanded. It doesn't create a "fake" file, so that shouldn't be used to determine whether something came from a macro or not.

So it should test for equality with the macro invocation span?

You test for equality modulo syntax context (i.e. sp1.lo == sp2.lo && sp1.hi == sp2.hi). You can't just equality check the spans since the reparsed tokens' syntax contexts will have an extra mark.

In general, if you want to check if something is emitted from a macro or not, you should check if the syntax context is not SyntaxContext::empty().

Ah, good advice there. We wanted this to return None if the token was produced by a macro, so that should work.

@jseyfried It looks like tokens produced by TokenStream::from_str() are given a non-empty SyntaxContext, so that doesn't quite work for that. Is that a bug in that impl?

Nevermind, I think I've figured out how to reliable deduce this.

aidanhs · 2017-08-16T23:51:59Z

@abonander are you looking for some feedback on this PR, or are you still working on it? I note the travis build has failed.

abonander · 2017-08-16T23:55:35Z

@aidanhs Looking for a response to #43604 (comment) since that would dictate what semantics the failing test would actually need to test for. It may necessitate touching more internals to set a different filename for the parsed TokenStream. Also unclear what source file DUMMY_SP should return, though it could just test equality and return None (that may provide a hacky way to detect the issue in the comment as well).

abonander · 2017-08-18T03:19:15Z

@jseyfried I got Span::source_file() working as expected, the semantics I was able to nail down are verified by this test. Is this something we can be happy with?

abonander · 2017-08-22T19:34:17Z

@jseyfried This is ready for review, I think.

LukasKalbertodt

I thought I could just try and do a code review. Note that I don't really know anything about compiler internals (although this might be a good thing? maybe? 😄). So maybe all comments are really useless ^_^

LukasKalbertodt · 2017-08-26T11:27:49Z

src/libproc_macro/lib.rs

+            return None
+        }
+
+        let loc = __internal::lookup_char_pos(self.0.lo);


Is it guaranteed that lo and hi point to the same file? In other words: are there spans which span multiple files? If there aren't, a comment explaining this would be nice for people reading the code and not knowing stuff about compiler internals (like me 😛). If there are, the doc comment should mention that the file of the start of the span is returned.

You know, because of how the compiler basically sticks an entire crate's source code together, you could potentially have a span that covers multiple files if the macro invocation contains mod declarations and if you glue() the tokens from the individual spans together.

So really start() and end() should probably return their own SourceFile's, but that might be exposing too much of the compiler's implementation details (because if it stopped doing that then the API would be misleading, but it would still work). Either that or glue() should not allow spans with different source files to be glued together.

If you glue the spans from the individual tokens together, that is. I'm on mobile for most of the day.

What about having something like BytePos in proc_macro? Then start() and end() would return a BytePos. And there are methods like BytePos::line() and BytePos::source_file(). I think I'd like that API better; what do you think?

LukasKalbertodt · 2017-08-26T11:29:58Z

src/libproc_macro/lib.rs

+        // If this span's token was produced by a macro
+        if self.0.ctxt != SyntaxContext::empty() {
+            return None
+        }


Is it somehow possible to recursively traverse up the macro call hierarchy? To get the span of the macro-invocation that generated the self span? Does this question make sense? 😕

It is possible, yes. I believe that's done for regular macros.

So should it be done here? If not, it should be possible to do the recursive traversal in user space IMO (which is not possible right now, right?).

I'm still not sure what you mean. There's already Span::call_site() which gets the span of the current macro invocation. I don't think we get much out of giving the macro call information about other macros than itself.

Oh sorry, I missed call_site(). That's exactly what I meant! :) Nevermind then.

LukasKalbertodt · 2017-08-26T11:33:38Z

src/libproc_macro/lib.rs

+
+    /// Create a new span encompassing `self` and `other`.
+    #[unstable(feature = "proc_macro", issue = "38356")]
+    pub fn glue(&self, other: Span) -> Span {


Is glue() a name already used today? Maybe hull() is more fitting?

Oh and regarding your glue_many() idea: I wouldn't make it a method, but an associated function:

fn hull_of(spans: &[Span]) -> Span

hull is a bit arcane for terminology. syntax_pos's Span uses to() but I feel like that's too short and undescriptive for a public API. Maybe to_other(), or Span::containing(one, another)?

As for glue_many(), it's trivially implementable in client code so I'd forget about it.

I think the term convex hull is actually the mathematically correct one here (although usually, 1D convex hulls are not that interesting). So hull() makes sense and is short. I'm also fine with Span::containing(), but it's longer than hull(). I don't like to() or to_other(). Or what about join()?

It's too much to explain in a short piece of documentation. We're looking for something that immediately makes sense.

join() isn't terrible, though.

LukasKalbertodt · 2017-08-26T11:39:37Z

src/libproc_macro/lib.rs

+impl SourceFile {
+    /// Get the underlying file name/path.
+    #[unstable(feature = "proc_macro", issue = "38356")]
+    pub fn as_str(&self) -> &str {


Does this return the path or the name of the file? If path: relative to the manifest-file? Relative to something else? Or absolute?

And if a path is returned, why not std::path::Path instead of &str? And then I'd also rename the method to path().

The underlying representation is a string but it is typically a path, yeah. I believe it's relative to the working directory for the compiler process.

It's relative to the directory containing the file being compiled, i.e. ~/foo/src for rustc ~/foo/src/lib.rs.
I think this method should be named path and should return an absolute path.

@jseyfried Are you want it to be absolute? It'd be more difficult to unit-test.

Perhaps the unit test could just check that a suffix of the absolute path is correct. I think it's worth having an absolute path so that the proc macro author doesn't have to worry about to which the path is relative.

bors · 2017-08-31T03:56:14Z

☔ The latest upstream changes (presumably #44186) made this pull request unmergeable. Please resolve the merge conflicts.

abonander · 2017-09-02T07:29:31Z

@jseyfried So it would seem that FileMap.name actually provides an absolute path anyway, though it might be a good idea to verify this in a few different contexts because maybe it relies on the CWD or something.

abonander · 2017-09-02T08:19:23Z

src/libproc_macro/lib.rs

+
+        let loc = __internal::lookup_char_pos(self.0.lo());
+
+        PathBuf::from(&loc.file.name).canonicalize().ok()


This could actually return &Path if we don't care about canonicalization.

Hmm, I would lean against canonicalizing, but I don't have that strong of an opinion.

Well, it'll save an allocation to skip it so that's good enough for me.

abonander · 2017-09-02T17:53:50Z

Build failure appears to be spurious, the ALLOW_PR build passed.

abonander · 2017-09-06T19:45:30Z

src/libproc_macro/lib.rs

+
+        let loc = __internal::lookup_char_pos(self.0.lo());
+
+        Some(Path::new(&loc.file.name))


@jseyfried That's right, this borrow is tied to loc instead of self so we can't return &Path here. Should I put it back to returning an opaque wrapper that can be dereferenced to Path, or just return a copy in PathBuf?

I think returning a copy in a PathBuf is fine, but I think the return value should be a newtype anyway for future compatibility.

Well if we return a newtype then getting &Path from it is as easy as returning PathBuf. Is a PathBuf still preferable in case we change the internals such that it's not easy to get a borrow that lives long enough?

So long as it's future compatible (i.e. just impl Deref { type Target = Path; }) it's up to you -- if internals change we can change later.

abonander · 2017-09-06T23:52:54Z

@jseyfried:

@LukasKalbertodt mentioned the real possibility that a span's start and end positions could be in different source files, given that the compiler expands mod declarations inline before processing attributes. For example (with foo/bar.rs being a separate file):

#[my_attr]
mod foo {
    fn foo();

    mod bar;
}

The my_attr invocation would see tokens with spans from two different source files:

mod foo {
    // tokens from the current file

    mod bar {
        // tokens from `foo/bar.rs`
    }
}

This only becomes an issue if two spans are joined that have different source files (as a source token wouldn't have this quirk). Do we want to prohibit this by having Span::join() return Option? Or have Span::start() and Span::end() return SourceFiles as well (which better matches the underlying codemap)?

abonander · 2017-09-06T23:54:03Z

@jseyfried Also, do the tests look good enough or do you want me to test the return values of Span::start() and Span::end()?

jseyfried · 2017-09-30T20:36:20Z

IMO, that hack is that we construct "fake" files named <{} macros> in the first place -- will be fixed with inter-crate spans.

abonander · 2017-09-30T20:38:37Z

Hacks beget hacks. Poetic.

abonander · 2017-09-30T21:38:10Z

src/libproc_macro/lib.rs

+        // This is a hack until intercrate spans are implemented and we can have real source files
+        // for spans generated in external macros.
+        // https://github.com/rust-lang/rust/pull/43604#issuecomment-333334368
+        self.filemap.name.ends_with("macros>")


Oh duh, this condition needs to be inverted.

abonander · 2017-10-01T05:05:29Z

Rebased against a significantly more recent master and fixed the test. If the build passes I'll squash this mess down.

philipc · 2017-10-01T05:08:31Z

src/libproc_macro/lib.rs

+    /// [`is_real`]: #method.is_real
+    # [unstable(feature = "proc_macro", issue = "38356")]
+    pub fn as_path(&self) -> &Path {
+        Path::new(&self.filemap.name)


Will this path be used to open files? Note that filemap.name is not a real path when filemap.name_was_remapped is true (using -Z remap-path-prefix-*). It will look like a real path though, so if it's just used for display purposes then that's fine.

So is_real() should probably return filemap.is_real_file() && filemap.name_was_remapped?

If it needs to be a real path that can be opened, then maybe you need the filemap.path field that I'm adding in #44940. If users of this API need to be able to open the file, then they need to be able to open the file even when using -Z remap-path-prefix-*.

The main use case is to refer to files in custom error messages, so they should make sense to the user.

Ok, in that case filemap.name is correct, and you don't need to check name_was_remapped. Maybe it should be returning a str instead of a Path, so that users are less likely to wrongly open it.

I assume it's used by some build systems. I do mention it in the as_str() docs, think it needs to be duplicated on is_real()?

The remapping is for when the source package is going installed at a location that is different from where it is built. eg I may compile in /home/philipc, but the source package will be installed in /usr/src. It's also useful for reproducible builds. See #38322 for the feature request.

What it means is that the paths in the output binaries should use the remapped location (such as in debuginfo or panic messages). We're also using the remapped path for error messages during compilation, but I'm less sure about why that is useful (maybe it's just for anonymization).

But if you need to open a file during compilation then you need to use the path that isn't remapped, so maybe we need to add an API for that too. Otherwise we'll get into a situation where crates will fail to compile when remapping is used.

Thanks for explaining! I guess we really should add an API to retrieve the original path in long term. The remap-flag should never break code that compiled before, I guess. But maybe this can be done later, as this PR has seen already quite a bit of iteration?

@philipc something just occurred to me: the way it's currently implemented, remapping seems to be an all-or-nothing thing, so can it just be a global flag instead of per Filemap?

It's not all-or-nothing: it's possible the remap prefix will only match some files, or there may be multiple prefixes that match different files. I don't know if this will happen in practice, but the compiler currently supports it.

philipc · 2017-10-01T05:08:37Z

src/libproc_macro/lib.rs

+        // This is a hack until intercrate spans are implemented and we can have real source files
+        // for spans generated in external macros.
+        // https://github.com/rust-lang/rust/pull/43604#issuecomment-333334368
+        !self.filemap.name.ends_with("macros>")


I'm not sure exactly which paths need to be flagged as not real, but note that there is a FileMap::is_real_file that only checks for '<' and '>'.

abonander · 2017-10-01T07:43:42Z

@philipc @jseyfried build passing with latest changes, can rebase whenever.

jseyfried · 2017-10-02T20:11:52Z

r=me with commits squashed.

abonander · 2017-10-02T21:05:27Z

I almost don't want to merge this, as of right now it's the oldest still-open PR 😄

abonander · 2017-10-02T21:10:03Z

@jseyfried rebased, much cleaner now

kennytm · 2017-10-05T07:13:07Z

@bors r=jseyfried

bors · 2017-10-05T07:13:08Z

📌 Commit c66c990 has been approved by jseyfried

bors · 2017-10-05T08:53:48Z

⌛ Testing commit c66c9903e4e44fa322bd76b2a37da7dc2574ba7d with merge fddec2dec4783140b06ffba217dad25bf215725b...

bors · 2017-10-05T10:50:08Z

💔 Test failed - status-travis

kennytm · 2017-10-05T10:53:12Z

@bors r-

run-pass-fulldeps/proc-macro/span-api-tests.rs failed on x86_64-gnu-aux, legit.

[01:50:20] failures:
[01:50:20] 
[01:50:20] ---- [pretty] run-pass-fulldeps/proc-macro/span-api-tests.rs stdout ----
[01:50:20] 	
[01:50:20] error: pretty-printed source does not typecheck
[01:50:20] status: exit code: 101
[01:50:20] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zno-trans" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps/proc-macro/span-api-tests.pretty-out" "--target=x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps/proc-macro/span-api-tests.stage2-x86_64-unknown-linux-gnu.pretty.libaux" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
[01:50:20] stdout:
[01:50:20] ------------------------------------------
[01:50:20] 
[01:50:20] ------------------------------------------
[01:50:20] stderr:
[01:50:20] ------------------------------------------
[01:50:20] error: proc macro panicked
[01:50:20]   --> <anon>:26:1
[01:50:20]    |
[01:50:20] 26 | assert_source_file! ("Hello, world!");
[01:50:20]    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:50:20]    |
[01:50:20]    = help: message: Source file is not real: SourceFile { path: "<anon>", is_real: false }
[01:50:20] 
[01:50:20] 
[01:50:20] ------------------------------------------
[01:50:20] 
[01:50:20] thread '[pretty] run-pass-fulldeps/proc-macro/span-api-tests.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2433:8
[01:50:20] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:50:20] 
[01:50:20] 
[01:50:20] failures:
[01:50:20]     [pretty] run-pass-fulldeps/proc-macro/span-api-tests.rs
[01:50:20] 
[01:50:20] test result: �[31mFAILED�(B�[m. 79 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

abonander · 2017-10-05T12:29:43Z

So the assertion is technically correct, that's not a real source file, but it doesn't really help us here. @jseyfried should I fall back to checking if the source file ends in macros>? Or should we somehow work around this test?

jseyfried · 2017-10-05T21:39:37Z

@abonander I believe adding an // ignore-pretty comment to the test will fix this.

abonander · 2017-10-06T00:01:09Z

@jseyfried amended with // ignore-pretty

abonander · 2017-10-06T08:17:36Z

@kennytm would you please re-r+

kennytm · 2017-10-06T13:36:51Z

@bors r=jseyfried

bors · 2017-10-06T13:36:53Z

📌 Commit 7be36d2 has been approved by jseyfried

bors · 2017-10-06T18:52:37Z

⌛ Testing commit 7be36d2 with merge 05cbece...

@jseyfried

Improvements to `proc_macro::Span` API Motivation: https://internals.rust-lang.org/t/better-panic-location-reporting-for-unwrap-and-friends/5042/12?u=logician TODO: - [x] Bikeshedding/complete API - [x] Implement tests/verify return values cc @jseyfried @nrc

bors · 2017-10-06T23:21:23Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing 05cbece to master...

rust-highfive assigned alexcrichton Aug 2, 2017

rust-highfive assigned jseyfried and unassigned alexcrichton Aug 2, 2017

abonander commented Aug 2, 2017

View reviewed changes

abonander force-pushed the proc_macro_span_api branch 2 times, most recently from 6ef8080 to 5644028 Compare August 2, 2017 03:45

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2017

abonander commented Aug 4, 2017

View reviewed changes

arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2017

abonander commented Aug 12, 2017

View reviewed changes

alexcrichton mentioned this pull request Aug 25, 2017

Allow access to location information from proc_macro TokenStreams #38546

Open

LukasKalbertodt reviewed Aug 26, 2017

View reviewed changes

abonander commented Sep 2, 2017

View reviewed changes

abonander commented Sep 6, 2017

View reviewed changes

abonander force-pushed the proc_macro_span_api branch from 4716f5c to decdf7d Compare September 7, 2017 07:26

abonander commented Sep 30, 2017

View reviewed changes

abonander force-pushed the proc_macro_span_api branch from 955261b to 4debb56 Compare October 1, 2017 05:04

philipc reviewed Oct 1, 2017

View reviewed changes

abonander force-pushed the proc_macro_span_api branch from 2fa99b0 to c66c990 Compare October 2, 2017 21:09

proc_macro::Span API improvements

7be36d2

abonander force-pushed the proc_macro_span_api branch from c66c990 to 7be36d2 Compare October 6, 2017 00:01

bors merged commit 7be36d2 into rust-lang:master Oct 6, 2017

dtolnay mentioned this pull request Dec 30, 2017

Tracking issue for RFC 1566: Procedural macros #38356

Closed

31 tasks


		let loc = __internal::lookup_char_pos(self.0.lo());

		PathBuf::from(&loc.file.name).canonicalize().ok()


		let loc = __internal::lookup_char_pos(self.0.lo());

		Some(Path::new(&loc.file.name))

Improvements to proc_macro::Span API #43604

Improvements to proc_macro::Span API #43604

Conversation

abonander commented Aug 2, 2017 • edited Loading

rust-highfive commented Aug 2, 2017

abonander commented Aug 2, 2017

Choose a reason for hiding this comment

abonander commented Aug 2, 2017

est31 commented Aug 2, 2017

abonander commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented Aug 16, 2017

abonander commented Aug 16, 2017 • edited Loading

abonander commented Aug 18, 2017 • edited Loading

abonander commented Aug 22, 2017

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasKalbertodt Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

abonander Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasKalbertodt Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

LukasKalbertodt Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 31, 2017

abonander commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander commented Sep 2, 2017

Choose a reason for hiding this comment

jseyfried Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander commented Sep 6, 2017 • edited Loading

abonander commented Sep 6, 2017

jseyfried commented Sep 30, 2017

abonander commented Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

abonander commented Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improvements to `proc_macro::Span` API #43604

Improvements to `proc_macro::Span` API #43604

abonander commented Aug 2, 2017 •

edited

Loading

abonander commented Aug 16, 2017 •

edited

Loading

abonander commented Aug 18, 2017 •

edited

Loading

abonander Aug 26, 2017 •

edited

Loading

LukasKalbertodt Aug 26, 2017 •

edited

Loading

abonander Aug 27, 2017 •

edited

Loading

LukasKalbertodt Sep 3, 2017 •

edited

Loading

abonander Aug 27, 2017 •

edited

Loading

LukasKalbertodt Sep 3, 2017 •

edited

Loading

abonander Aug 27, 2017 •

edited

Loading

jseyfried Sep 6, 2017 •

edited

Loading

abonander commented Sep 6, 2017 •

edited

Loading

abonander commented Sep 30, 2017 •

edited

Loading

abonander commented Oct 1, 2017 •

edited

Loading

abonander Oct 1, 2017 •

edited

Loading

abonander commented Oct 5, 2017 •

edited

Loading

jseyfried commented Oct 5, 2017 •

edited

Loading