Coverage by svkf · Pull Request #303 · icecc/icecream

svkf · 2017-10-18T03:21:05Z

add extra_file support for code coverage and save_temps
merge split dwarf into that setup
should be backward compatible
added tests for various combinations of the extra files

and a few log messages reowkr the -pipe in conjunction with -save-temps

though need to get rid of all using namespace std is better but for another commit

HenryMiller1 · 2017-10-18T15:06:25Z

 -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes \
 -Wnested-externs $CFLAGS"
-    CXXFLAGS=" -g -W -Wall -Wpointer-arith $cast_align $wshadow -Wwrite-strings $CXXFLAGS"
+    CXXFLAGS=" -std=gnu++11 -g -W -Wall -Wpointer-arith $cast_align $wshadow -Wwrite-strings $CXXFLAGS"


This is okay, but I would prefer a minimum level of this, while allowing higher standards if the compiler supports it... (I'm working on cmake which handles this easier than autotools this so don't worry about it)

yeah just the code get so much nicer ... for the for () loops
I can rewrite it the old way ... and remove if you want

No, I'm saying that if C++14 is available I want to use that instead of C++11.

I'm not sure how far back we want to support C++ standards, maybe the next release should require C++11, and C++17 for the one after that?

My vote would be c++ 14 for the next release, and 17 after that. For what it's worth, I've seen several areas of code that could profit from string_view in C++ 17.

HenryMiller1 · 2017-10-18T15:06:57Z

    myaddr.sun_family = AF_UNIX;

-    mode_t old_umask = -1U;
+    mode_t old_umask = (mode_t)-1U;


odn't use C style cases - I think you want static_cast here?

yes, personally I jut hate the extra typing :)

HenryMiller1 · 2017-10-18T15:08:06Z

        bool hasPipe = false;
+        bool hasSaveTemps = false;

+        // we could ignore this, except the silly gcc-4.8 thing with tthe caret fails the tests due


tthe - fix the typo in this comment

HenryMiller1 · 2017-10-18T15:08:27Z


+        // we could ignore this, except the silly gcc-4.8 thing with tthe caret fails the tests due
+        // to having -pipe and -save-temps=obj
+        for (std::list<string>::const_iterator it = list.begin();


why didn't you use auto here?

just copied the line below it ...
was sort of last minute, I didn't have this fix in my original since I don't use 4.8 normally but it happened to be on the box I was using

problem was the test failed since it got some stderr and the 4.8 thing
but didn't actually error (even with -Werror), normally for my setup it just kept going ...
gcc: warning: -pipe ignored because -save-temps specified
I was looking at the fix in my setup to just clean it up but figured its so rarely used ... I didn't care (till the test failed)

HenryMiller1 · 2017-10-18T15:15:04Z

It looks good on first glance. I see some minor style issues. I'll need to look at it a little longer, but thanks for the work.

I'll want @johnmiked15 to look at it too to make sure I'm not missing something, if he is still around

HenryMiller1 · 2017-10-18T15:15:54Z

This fixes #187

svkf · 2017-10-18T15:28:33Z

one issue is the API
the 38 is for all extra_files but then there is an implicit API on the enum/string too
so technically on any new extra_files you would have to raise the number even though the messaging stayed the same
if you don't a new client (-fxxxx) would ask for 38 and if old server (38 but not with new enum) it would fail to get that data
or a old client may use some -fxxx and the remote would look for it (38 with new enum) and send back
but the client wouldn't actually be expecting it since it wouldn't loop through that next one ...

not sure how to handle that .. (just bump API on new enum?) and ensure checked in minVersion?
which is consistent
could not use enum and tack extra bytes for each enum ... but then that would be more code each change which I don't like

and static cast on broken mode_t for mac/c++ vs C-cast

HenryMiller1 · 2017-10-18T16:21:45Z

Would it be possible to send to the remote a list of files to send back? Then the version 38 clients are compatible with new file types. Otherwise just put a comment by the enum that you need to update the protocol version when the enum changes.

svkf · 2017-10-18T16:38:57Z

unfortunately that doesn't help
(new client, old server)
you send a,b,c it only can send back a,b
so it would say pass ... but wouldn't actually work the way you want
the scheduler needs to know which guy to pick for what you want
I could add to the scheduler message GetCSMsg() (in 38) the max enum, then it can use that plus the API#
then its just automatic and no updates needed

(I did something myself adding a custom/API version so that if I added messages on my own I don't pollute the API space on new update from git
(ie naming users, user stats etc.)

HenryMiller1 · 2017-10-27T01:53:46Z

    job.setFlags(args);
    job.setOutputFile(ofile);
+    if (seen_split_dwarf) {
+        // need to do after the output file is set */


remove the c style comment end - this is a C++ comment

HenryMiller1 · 2017-10-27T02:02:31Z

+	    abort_tests
+	fi
+    fi
+    # note thiese differ a bit based on the args passed to compiler


spelling - thiese-> these

HenryMiller1 · 2017-10-27T02:03:02Z

+
+    void setExtraOutputFile(uint32_t index, const std::string &file)
+    {
+        //printf("setExtraOutputFile[%d] = %s\n", index, file.c_str());


dead code, please remove, or use the standard logging means to log this

will do these, just been away

HenryMiller1

3 comments need tweaking. Since nobody else has look at this in a week I'll merge as soon as they are fixed unless someone sees something wrong I don't.

dantje · 2018-01-20T16:22:30Z

I tried to rebase these commits against master in (https://github.com/dantje/icecream/commits/coverage) and got this to compile and at least it claims to transmit gcno files.

deriamis · 2022-06-12T19:09:35Z

I'm going to look into this during the next week or so and see if I can revive it. At this point (years later), I think we should just go for C++17, so I'll see if I can go in that direction - though I would like to keep GNU extensions out of it if I can.

svkf added 5 commits October 17, 2017 08:58

add support for extra_files

25f733b

make a few changes for the protocol 38

cee58f7

and a few log messages reowkr the -pipe in conjunction with -save-temps

update a few variables that shadowed others (noticed in travis build)

809a9cc

fixup issues with mac c++

45da679

though need to get rid of all using namespace std is better but for another commit

fixup more bind issues

ca30b0f

HenryMiller1 reviewed Oct 18, 2017

View reviewed changes

HenryMiller1 assigned jdrouhard Oct 18, 2017

update typo in comment

b67a60d

and static cast on broken mode_t for mac/c++ vs C-cast

HenryMiller1 reviewed Oct 27, 2017

View reviewed changes

HenryMiller1 requested changes Oct 27, 2017

View reviewed changes

HenryMiller1 added 2 commits October 31, 2017 11:08

Merge branch 'master' into coverage

7fd5057

Merge branch 'master' into coverage

dd748ea

llunak mentioned this pull request Dec 28, 2017

Compiling with -fprofile-generate #156

Closed

update to latest protocol and fix other issues

3d2f477

jdrouhard removed their assignment Mar 5, 2018

deriamis self-assigned this Jun 12, 2022

Conversation

svkf commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HenryMiller1 commented Oct 18, 2017

Uh oh!

HenryMiller1 commented Oct 18, 2017

Uh oh!

svkf commented Oct 18, 2017

Uh oh!

HenryMiller1 commented Oct 18, 2017

Uh oh!

svkf commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HenryMiller1 left a comment

Choose a reason for hiding this comment

Uh oh!

dantje commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deriamis commented Jun 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dantje commented Jan 20, 2018 •

edited

Loading