Skip to content

core/list: add remove method#6211

Merged
miri64 merged 3 commits intoRIOT-OS:masterfrom
lebrush:list_remove
Jan 12, 2017
Merged

core/list: add remove method#6211
miri64 merged 3 commits intoRIOT-OS:masterfrom
lebrush:list_remove

Conversation

@lebrush
Copy link
Copy Markdown
Member

@lebrush lebrush commented Dec 14, 2016 •
edited by haukepetersen
Loading

this adds a method to remove elements from a list

Required by: #6158

@lebrush lebrush mentioned this pull request Dec 14, 2016
Merged
@kaspar030 kaspar030 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Dec 14, 2016
@kaspar030 kaspar030 self-assigned this Dec 14, 2016
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 14, 2016
@miri64 miri64 requested a review from kaspar030 December 14, 2016 11:14
miri64
miri64 previously approved these changes Dec 14, 2016
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK when Murdock is happy and PR received second ACK

@miri64 miri64 dismissed their stale review December 14, 2016 11:17

Ooops, can you please add some unittests to tests/unittests/tests-core (I know… there are none for list.h yet. All the more reason to add some)

miri64
miri64 requested changes Dec 14, 2016
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

.

miri64
miri64 requested changes Dec 14, 2016
View reviewed changes
core/include/list.h Outdated
}
list = list->next;
}
return NULL;
Copy link
Copy Markdown
Member

@miri64 miri64 Dec 14, 2016

Choose a reason for hiding this comment

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

No import of <stddef.h> in this file (yet ;-)), so NULL is not defined (found by Murdock)

Copy link
Copy Markdown
Member Author

@lebrush lebrush Dec 14, 2016

Choose a reason for hiding this comment

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

Instead of adding the include, I used the same strategy as in the other methods, returning list->next knowing that it will be NULL.

@miri64 miri64 self-assigned this Dec 14, 2016
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Dec 14, 2016

@miri64 I don't have time to write unittests for it now. I'll open an issue for that.
I'll add the include quickly this afternoon :-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 14, 2016

KK.

miri64
miri64 approved these changes Dec 14, 2016
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK and go as soon as Murdock and a second reviewer are happy.

kaspar030
kaspar030 approved these changes Dec 14, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Thanks @lebrush!

This was referenced Dec 14, 2016
Merged
Open
@lebrush lebrush mentioned this pull request Dec 15, 2016
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 15, 2016

Arghs, damn emb6 and its non-specific naming. Will provide a fix...

miri64 added a commit to miri64/RIOT that referenced this pull request Dec 15, 2016
@miri64
emb6: prepare for RIOT-OS#6211
fcf072c
@miri64 miri64 mentioned this pull request Dec 15, 2016
Merged
miri64 added a commit to miri64/RIOT that referenced this pull request Dec 15, 2016
@miri64
emb6: prepare for RIOT-OS#6211
6c90972
miri64 added a commit to miri64/RIOT that referenced this pull request Dec 15, 2016
@miri64
emb6: prepare for RIOT-OS#6211
99320eb
miri64 added a commit to miri64/RIOT that referenced this pull request Dec 15, 2016
@miri64
fixup! emb6: prepare for RIOT-OS#6211
16571f3
miri64 added a commit to miri64/RIOT that referenced this pull request Jan 10, 2017
@miri64
emb6: prepare for RIOT-OS#6211
185be4e
@OlegHahm OlegHahm added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 10, 2017
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 10, 2017

#6225 was merged

miri64 added a commit that referenced this pull request Jan 10, 2017
@miri64
Merge pull request #6225 from miri64/pkg/fix/emb6-prep-6211
94b7c08 
emb6: prepare for #6211
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 10, 2017

(please rebase)

@miri64 miri64 added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 10, 2017
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 10, 2017

((and squash ;-))

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 11, 2017
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Jan 11, 2017

@miri64 rebased! if I squash then I touch irrelevant lines for the remove method in the same commit, I don't think it is a good idea...

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 11, 2017

kk

@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Jan 11, 2017

ahhhrrrggg not again! nhdp has a list_remove... I'll check into within today 🎉

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 11, 2017

It's oonf_api, not nhdp ;-)

miri64
miri64 approved these changes Jan 11, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Though I'm not happy with including the changes to oonf here, I guess you just want to get done, so re-ACK

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 11, 2017
bergzand pushed a commit to bergzand/RIOT that referenced this pull request Jan 11, 2017
@miri64 @bergzand
emb6: prepare for RIOT-OS#6211
f6b27af
bergzand pushed a commit to bergzand/RIOT that referenced this pull request Jan 11, 2017
@miri64 @bergzand
emb6: prepare for RIOT-OS#6211
bd05ff7
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Jan 12, 2017

Can someone remove the NEEDS SQUASHING to make Murdock happy, please :-)

Running './dist/tools/pr_check/pr_check.sh master' x
Command output:

	Pull request needs squashing according to its labels set on GitHub

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 12, 2017
@miri64 miri64 merged commit 51d13ee into RIOT-OS:master Jan 12, 2017
@lebrush lebrush deleted the list_remove branch January 12, 2017 12:18
@lebrush
Copy link
Copy Markdown
Member Author

lebrush commented Jan 12, 2017

🎉 thx!

kb2ma pushed a commit to kb2ma/RIOT that referenced this pull request Jan 14, 2017
@miri64 @kb2ma
emb6: prepare for RIOT-OS#6211
9df2b9e
neiljay pushed a commit to neiljay/RIOT that referenced this pull request Jan 16, 2017
@miri64
emb6: prepare for RIOT-OS#6211
9f8a203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@kaspar030 kaspar030 kaspar030 approved these changes

Assignees

@miri64 miri64

@kaspar030 kaspar030

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lebrush @miri64 @kaspar030 @OlegHahm