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

Some minor uemis improvements #47

Closed
wants to merge 2 commits into from
Closed

Some minor uemis improvements #47

wants to merge 2 commits into from

Conversation

dirkhh
Copy link
Collaborator

@dirkhh dirkhh commented Oct 1, 2011

Allow passing SDA files on the command line, just like xml files.

Basic error handling in the uemis parser (don't just fail quietly).

Initial support for uemis events (I picked the Marker as first one, many
more to follow).

Small bugfixes.

Signed-off-by: Dirk Hohndel dirk@hohndel.org

Allow passing SDA files on the command line, just like xml files.

Basic error handling in the uemis parser (don't just fail quietly).

Initial support for uemis events (I picked the Marker as first one, many
more to follow).

Small bugfixes.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
@torvalds
Copy link
Collaborator

torvalds commented Oct 1, 2011

On Sat, Oct 1, 2011 at 8:24 AM, Dirk Hohndel
reply@reply.github.com
wrote:

Allow passing SDA files on the command line, just like xml files.

This is wrong.

You can't use "strstr()" for something like that. You need to check
the end of the string, not a 'does it contain ".SDA" anywhere in the
string' test.

Right now you do crazy things for "Hello.SDAG.xml" and just say "it
contains the string '.SDA' so I think it's an SDA file"

               Linus

@dirkhh
Copy link
Collaborator Author

dirkhh commented Oct 1, 2011

Any algorithm that triggers on the filename is bound to FAIL - but you are right, this was a quick hack that I should have cleaned up.
Will do later ;-)

/D

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Linus Torvalds reply@reply.github.com wrote:

On Sat, Oct 1, 2011 at 8:24 AM, Dirk Hohndel
reply@reply.github.com
wrote:

Allow passing SDA files on the command line, just like xml files.

This is wrong.

You can't use "strstr()" for something like that. You need to check
the end of the string, not a 'does it contain ".SDA" anywhere in the
string' test.

Right now you do crazy things for "Hello.SDAG.xml" and just say "it
contains the string '.SDA' so I think it's an SDA file"

Linus

Reply to this email directly or view it on GitHub:
https://github.com/torvalds/subsurface/pull/47#issuecomment-2257409

This should work; and yes, not ignoring case is intentional. These should
be ".SDA" files, not ".sda"

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
@dirkhh
Copy link
Collaborator Author

dirkhh commented Oct 1, 2011

Intentionally only accepting ".SDA" files...

@torvalds
Copy link
Collaborator

torvalds commented Oct 1, 2011

On Sat, Oct 1, 2011 at 9:26 AM, Dirk Hohndel
reply@reply.github.com
wrote:

Intentionally only accepting ".SDA" files...

I'm not taking crap like this.

Seriously.

strstr is the wrong thing to do. Don't use it. Don't even save the old
broken commit that did it. It's crap. It was wrong before - but it's
EVEN MORE WRONG NOW. And you still use it.

Now it happens to work because you limit it to the end of the string,
but christ it's stupid. I look at that code and I start looking for
spoons to dig out my eyeballs with.

The function you want is called "strcmp". You compare one exact
string against another. You don't try to "find" one string inside
another. What you are trying to do has nothing to do with strstr.

I have to say, I'd also suggest you keep the "event" logic as a
separate commit from the code that changes the interface. The two have
nothing to do with each other.

                  Linus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants