-
Notifications
You must be signed in to change notification settings - Fork 41
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
Improve example code #237
Improve example code #237
Conversation
- Enable OpenMP where needed - Add some extra warning flags - Adds .gitignore to ignore the compiled binaries
@for bin in $+; do \ | ||
echo -- $$bin ------------------------------; \ | ||
$(RUNCMD) $(RUNOPT) ./$$bin; \ | ||
echo -- exit status: $$?; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to reintroduce a statement to the effect: if [ $$? -ne 0 ]; then exit $$?; fi;
Otherwise, Travis will not be able to flag failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, we aren't testing these with Travis during spec builds. This is because newly introduced APIs aren't yet supported and can't be tested. This is still useful to have, because I think we do run these tests (e.g. for a specific tagged version of the spec) as part of automated testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this change is that make run
was failing on shmem_global_exit
, which intentionally returns a nonzero status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm .. we could introduce a TESTS_XFAIL
list to handle cases like this. The atomics clarification proposal also introduces three examples with undefined behavior, which we would want to exclude from running. So, maybe TESTS_SKIP
? Or, move to a proper testing harness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change may cause make run
to succeed even after one of the examples fails? I remember doing a bit of tinkering around that || $$?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead we could print PASS or FAIL (and the exit code)?
Here are some screenshots of the revised/reformatted examples (left) vs. the original/master examples (right). For the most part, page numbering and overall spacing is very similar, as expected. (removed outdated screenshots) |
This isn't quite ready. Some things I've noticed:
Regarding the pshmem_example.c, I prefer this more compact form which doesn't increment time for floating point casting and arithmetic. This should improve the timing accuracy. Not sure if this can be considered a doc edit. #include <pshmem.h>
#include <stdio.h>
#include <sys/time.h>
static double total_put_time = 0.0;
static double avg_put_time = 0.0;
static long put_count = 0;
void shmem_long_put(long *dest, const long *source, size_t nelems, int pe) {
struct timeval t0, t1;
gettimeofday(&t0, 0); /* start timer */
pshmem_long_put(dest, source, nelems, pe); /* name shifted call to put */
gettimeofday(&t1, 0); /* end timer */
/* increment statistical values */
total_put_time += t1.tv_sec - t0.tv_sec + 1e-6*(t1.tv_usec - t0.tv_usec);
put_count++;
avg_put_time = total_put_time / (double)put_count;
} |
@jamesaross I went through all the examples individually, and I think I've addressed all the items you noted. |
@nspark Thank you. The examples look improved. They'll never be perfect, so we'll probably periodically come back around to these. Some things to address at some point in the future:
|
@nspark This request is more about document usability... When you open the generated PDF and use a cursor to select/highlight the embedded example codes in order to copy the text, the line numbers on the left and right of the pages are also copied. This makes it difficult for a reader to copy/paste the example code into a text editor and use it without removing all the line numbers. This doesn't happen with the regular paragraph blocks. Is there a way to avoid this? I'm using the evince PDF reader on Linux. Maybe an alternative solution is to exclude line numbers on the final drafts? These were added in the 1.1 document in 2014 and I wasn't around then for the discussion. I don't find the PDF line numbers useful beyond collaborating on the draft documents. It's just clutter otherwise. |
@jamesaross 👍 on removing the page-level line numbers entirely. I don't find them helpful, and they don't even align to any particular line of text. |
@for bin in $+; do \ | ||
echo -- $$bin ------------------------------; \ | ||
$(RUNCMD) $(RUNOPT) ./$$bin; \ | ||
echo -- exit status: $$?; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change may cause make run
to succeed even after one of the examples fails? I remember doing a bit of tinkering around that || $$?
.
Within utils/defs.tex, I believe these keywords should be moved from the language keywords to the type keywords otherwise the code highlighting looks off to me. And also These should be added to the C keywords for future code: In, writing_shmem_example.c, there is a |
@jdinan Good to go! |
This PR (will):
-Wall -Wextra -pedantic -Werror
clang-format
Tagging @jamesaross and @jdinan for review.
Closes #230