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

Hyperlink fixes #1821

Merged
merged 3 commits into from Oct 29, 2015
Merged

Hyperlink fixes #1821

merged 3 commits into from Oct 29, 2015

Conversation

Bcorde5
Copy link
Contributor

@Bcorde5 Bcorde5 commented Oct 26, 2015

Fixed issues brought up in #1817 , please report any additional problems, and don't be afraid to ask questions, I am sure there will be some.

@@ -90,8 +92,6 @@ namespace boost
line_start = it + 1; // could be end()
}
}

std::string lineloc = linelink(full_path, std::to_string(line_number));
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? Shouldn't the line number be represented by a hyperlink?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line number should be represented by a hyperlink, however the structure of the inspect tool requires the line_number to be a size_t, so I implemented the switch in inspect.cpp for this particular case, when there are four arguments in the error report. If the ascii_check is not creating a proper hyperlink, I will have to check it out.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use this overload for all of the checkers, then? What's so special about apple_macro_check?

Also, where is that switch you're referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works for both apple_macro and ascii check, as both have four arguments, the last one having three. If the link works, https://github.com/Bcorde5/hpx/blob/HyperlinkFixes/tools/inspect/inspect.cpp#L547-L555 is where the overload occurs. This only works, and the only reason it must work is because in both of these checkers, the line number is not included in the string, but rather as a size_t argument. The other checkers cannot use a similar system because they either do not have a line number or the line number is within the string, which makes it easier to point out multiple mistakes in a single run on inspect, but makes it tricky creating such an overload for the other checkers with only three arguments.

@hkaiser
Copy link
Member

hkaiser commented Oct 26, 2015

The ascii_check checker still does not create the proper hyperlink for the given line number.

@Bcorde5 Bcorde5 force-pushed the HyperlinkFixes branch 2 times, most recently from d09a0b3 to aa3f217 Compare October 26, 2015 20:20
@hkaiser hkaiser added this to the 0.9.11 milestone Oct 27, 2015
@@ -1062,7 +1077,8 @@ void print_output(std::ostream& out, inspector_list const& inspectors)
"<tr>\n"
"<td>"
"<a href = \"https://github.com/STEllAR-GROUP/hpx\">"
"<img src=\"http://stellar.cct.lsu.edu/files/stellar100.png\" alt=\"STE||AR logo\" />"
"<img src=\"http://stellar.cct.lsu.edu/files/stellar100.png\"\
alt=\"STE||AR logo\" />"
Copy link
Member

Choose a reason for hiding this comment

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

Please don't break a quoted string in the middle by inserting a \<newline>. This is just confusing, Use compile time string concatination instead:

"<img src=\"http://stellar.cct.lsu.edu/files/stellar100.png\""
    " alt=\"STE||AR logo\" />" 

@hkaiser
Copy link
Member

hkaiser commented Oct 27, 2015

Because of the GIT hash not being available at compile time when compiling in CircleCI, we should also add a command line option to the inspect tool which allows to specify the hash to be used on the tool's command line. Something like:

inspect --git-hash=eda56f5255eb9a27a7e3d1f2daffe5a1ac3bbc1f ...

Internally, the inspect tool should use the HPX_HAVE_GIT_COMMIT constant by default, but prefer using the command line argument if given.

hkaiser added a commit that referenced this pull request Oct 29, 2015
@hkaiser hkaiser merged commit ca1368f into STEllAR-GROUP:master Oct 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants