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

Fix includes for histogram.h and reuse_distance.h #2810

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

snehasish
Copy link
Contributor

Remove relative paths for includes in histogram.h and reuse_distance.h.

@fhahn
Copy link
Contributor

fhahn commented Jan 18, 2018

run aarch64 tests

@fhahn
Copy link
Contributor

fhahn commented Jan 18, 2018

Thank you very much for your patch. Could you explain which problem this fixes? Also, more files of drcachesim still have relative imports, e.g. https://github.com/DynamoRIO/dynamorio/blob/master/clients/drcachesim/reader/file_reader.h

@snehasish
Copy link
Contributor Author

Apologies for the terse description.

This fix removes the relative paths so that when reusing code for internal tools at Google, we can specify includes via -I. At this time we are interested in exposing the histogram_t and reuse_distance_t tools only.

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Looks good. We're holding off on merges to master until the Mac build is fixed -- and that fix (#2809) is unfortunately still sitting waiting for the Travis Mac queue...

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

Great thanks! Seems the builds are fine, LGTM given @derekbruening does not have any objections

@derekbruening
Copy link
Contributor

This won't affect Mac so we'll put this one through as #2809 will have to re-run anyway after removing the Mac PR tweak.

@fhahn
Copy link
Contributor

fhahn commented Jan 18, 2018

Ok, @derekbruening are you doing the merge or should I? Not sure how github handles race conditions there :)

@derekbruening derekbruening merged commit 3148ec7 into DynamoRIO:master Jan 18, 2018
@derekbruening
Copy link
Contributor

I did the merge and change the commit msg to make it part of #2006.

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.

3 participants