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

Implement Heapsize calculations #31

Merged
merged 9 commits into from
Aug 20, 2018

Conversation

Michael-F-Bryan
Copy link
Contributor

I often like having the ability to track how much memory my compiler/parser is using. This PR implements HeapSizeOf from the heapsize crate for various data types in codespan, hidden behind an off-by-default feature flag.

Copy link
Owner

@brendanzab brendanzab left a comment

Choose a reason for hiding this comment

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

Nice stuff - thanks for the contribution!

I'd probably like to see this tested on Travis in a build matrix. An example of this can be seen with Moniker's .travis.yml. Note that I had to do an ugly workaround of this bug in Cargo rust-lang/cargo#4942 to get it to work though, as setting features in workspaces is pretty broken at the moment. 🤦‍♂️

@@ -4,6 +4,7 @@ use std::borrow::Cow;
use std::path::{Path, PathBuf};
use std::{fmt, io};

use heapsize::{self, HeapSizeOf};
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be cfg-ed out with a feature flag!

@@ -59,6 +60,27 @@ impl fmt::Display for FileName {
}
}

impl HeapSizeOf for FileName {
Copy link
Owner

Choose a reason for hiding this comment

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

This too!

@Michael-F-Bryan
Copy link
Contributor Author

Thanks for pointing out the missing #[cfg]'s @brendanzab. I had a look through the issue you linked and it seems like your previous solution is the best (or at rather, least bad) workaround.

.travis.yml Outdated
- nightly
- stable
- beta
- nightly
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: could we use consistent indentation here (sorry 😅 )

@brendanzab
Copy link
Owner

Looks good! I'm about to head to bed, but would you like me to merge and release tomorrow morning?

Also, we have a gitter room if you'd like to chat with us in a more real time setting too. Curious to know what you plan to do with codespan! :)

@Michael-F-Bryan
Copy link
Contributor Author

would you like me to merge and release tomorrow morning?

I'm currently importing directly from my git fork at the moment, but if you have time to do a release that'd be awesome!

I've used codespan for a couple other projects in the past, but none are really public. For a bit of fun I thought I'd write the beginnings of a C compiler, with codespan being used throughout the AST and for diagnostics. I can't say I've had any complaints so far, it's one of those libraries which integrates in seamlessly to the point that you don't actually notice it's 3rd party code 😀

@brendanzab brendanzab merged commit a18ec20 into brendanzab:master Aug 20, 2018
@brendanzab
Copy link
Owner

This is now released on crates.io!

@Michael-F-Bryan Michael-F-Bryan deleted the heapsize branch August 21, 2018 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants