Skip to content

ATLEDGE-553: Modify library examples header info #173

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

Closed
wants to merge 7 commits into from

Conversation

bokibi
Copy link

@bokibi bokibi commented Apr 29, 2016

No description provided.

@bokibi
Copy link
Author

bokibi commented Apr 29, 2016

Moved all Copyright header information from Library folder to separate COPYRIGHT_XXX.README files.

@calvinatintel
Copy link
Contributor

Hi Mike. We like the current style of license headers as it cleanly separates individual files with their respective licenses.

@calvinatintel
Copy link
Contributor

Hi Mike. How about this solution?

Put this on the top of the files:

/*
 * Copyright (c) 2015 Intel Corporation.  All rights reserved.
 * See the bottom of this file for the license terms.
 */

then put this on the bottom of the file:

/*
   Copyright (c) 2015 Intel Corporation.  All rights reserved.

   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2.1 of the License, or (at your option) any later version.

   This library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with this library; if not, write to the Free Software
   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
*/

Each file will contain the license terms for itself this way while keeping the top of the files cleaner.

@bokibi
Copy link
Author

bokibi commented May 3, 2016

Hi Calvin,

Normally, the Copyright notice/license appears on top of source code or in a separate LICENSE file. In this case, I've created separate LICENSE files as you can see in my check-ins. If you think we need to move the license term of each file to the bottom of it, I am happy to do so. Please confirm :)

@SidLeung
Copy link
Contributor

SidLeung commented May 3, 2016

@bokibi Understood the normal format of the Copyright / license notification. To comply with the internal legal protocol and the Sketch appearance requirement, it is desirable to employ the layout specified above by Calvin. Thank you.

@bokibi
Copy link
Author

bokibi commented May 3, 2016

Sure. Will do, Sid.

#include "BLEAttribute.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this whiteline

@calvinatintel
Copy link
Contributor

Sorry for the change request again. Can you please update the copyright year from 2015 to 2016?

@calvinatintel
Copy link
Contributor

@bokibi ping

@bokibi
Copy link
Author

bokibi commented May 17, 2016

Sure, @calvinatintel :)

@bokibi
Copy link
Author

bokibi commented May 25, 2016

Hey Calvin - I just updated the the Intel Copyright date. Please let me know if I missing anything :) Thx.

@calvinatintel
Copy link
Contributor

@bokibi @eriknyquist Can you please take a look at the conflict?

while (*p1 == *p2++)
if ('\0' == *p1++)
return 0;
return (*(const char *)p1 - *(const char *)(p2 - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Sneaking in a functional change ;)
I'm happy with this, usually not a fan of re-implementing std routines but in this case it does appear that
"strcmp" is indeed not behaving as advertised. I'll fix the conflict and merge it.

This seems odd to me since we are not using any home-made versions of strcmp, it just comes from the std. C++ lib (which, admittedly, comes with our toolchain). If there is an error in the libs shipped with our toolchain, I'd like to find it-- when was the first time you noticed this issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wuh.. this is not okay. I see a few more code changes in the PR. Perhaps the PR needs to be rebased to the tip?

Copy link
Contributor

@eriknyquist eriknyquist May 26, 2016

Choose a reason for hiding this comment

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

Oh yeah you're right, there are more functional changes that I missed. OK, so @bokibi can you please remove functional changes from this PR? leave header changes only, like the title suggests.

That said, I believe your "strcmp" re-implementation is a valid fix, and I would like to merge it also if we cannot fix the toolchain. Please make a new pull request for each functional change. And, like @calvinatintel says you'll probably need to rebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Calvin. It may require more work. However, for future referencing and categorizing, it is desirable to separate out issues into individual reports. It will also ensure the discussion, analysis of the issues will not be mixed together.

Copy link
Author

@bokibi bokibi May 26, 2016

Choose a reason for hiding this comment

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

​Agree. I will back out the change for strcmp issue. Erik, the issue was
found by Janet Y Scholar and assigned to me.

@kitsunami kitsunami added this to the Castor milestone May 31, 2016
@eriknyquist
Copy link
Contributor

eriknyquist commented Jun 2, 2016

There are 7 commits for this headers change, including 1 merge commit, 1 commit to revert the strcmp change, and 3 commits that are a mystery to me because they all have the same message. Can you clean it up please @bokibi ? rebase into a single commit for the header changes and remove all traces of strcmp changes.

@calvinatintel
Copy link
Contributor

@eriknyquist This is replaced with #206

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.

5 participants