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

Duplicate device instances are ignored #325

Open
m8pple opened this issue Jul 14, 2022 · 5 comments
Open

Duplicate device instances are ignored #325

m8pple opened this issue Jul 14, 2022 · 5 comments
Assignees
Labels
bug Report of a bug

Comments

@m8pple
Copy link
Contributor

m8pple commented Jul 14, 2022

If a graph contains devices with duplicate device instances, then one of them
is silently ignored rather than raising an error.

@m8pple
Copy link
Contributor Author

m8pple commented Jul 14, 2022

See PR #326 for test-case.

@mvousden
Copy link
Contributor

If a graph contains devices with duplicate device instances, then one of them is silently ignored rather than raising an error.

Yeah, that's not supposed to happen.

@DrongoTheDog thoughts?

@mvousden mvousden added the bug Report of a bug label Jul 15, 2022
@mvousden
Copy link
Contributor

On Sun, Jul 17, 2022 at 12:15:57PM +0100, Andrew Brown wrote:

You know, I have even have a vague recollection of this, but I thought
"Screw it, what's the harm in ignoring it?".

Also - philosophical point - the natuiral place to get this is in the
Validator, but it's not really a validation issue - you can't capture
name uniqueness in the validation grammar.

I think:

  1. It could/should be ignored; it have never have any adverse effects
    (other than it indictates the monkey has cocked up).

  2. It should therefore be posted as a Warning.

  3. The natural place to do this is by the conditional that traps it,
    but I don't have the time right now to hunt it.

  4. OK, you knew I would:

DevI_t * DS_XML::_DevI_t( .... )

My source, line 150:

if (pD->Def()!=0) { // Already defined?
fprintf(fd,"(%3u,-) W%3u: Device %s already defined on line %u\n",
pn->lin,++wcnt,dname.c_str(),pD->Def()); // Cockup. Bail.

So it is reported, in the microlog for the datastructure build, as a
build warning.

If you Post a warning to the LogServer, that's a bit more "in yer face",
but on the other hand if you do that you'll have to Post every other
microlog message for consistency, and that's exactly what the microlog
is there to avoid.

ADB

@mvousden
Copy link
Contributor

Agree, up until:

If you do [Post a warning to the LogServer] you'll have to Post every other
microlog message for consistency, and that's exactly what the microlog is
there to avoid.

We should really be posting that there are warnings, and point the user to
the microlog if so. Let the operator walk to their own destruction if they
wish, but at least we told them so.

Nobody reads the micrologs unless prompted, anyway (by design).

@mvousden
Copy link
Contributor

On Mon, Jul 25, 2022 at 11:01:22AM +0100, ADB wrote:

On 18/07/2022 10:01, Mark Vousden wrote:

Agree, up until:

If you do [Post a warning to the LogServer] you'll have to Post every other
microlog message for consistency, and that's exactly what the microlog is
there to avoid.
We should really be posting that there are warnings, and point the user to
the microlog if so. Let the operator walk to their own destruction if they
wish, but at least we told them so.
Fair point
Nobody reads the micrologs unless prompted, anyway (by design).
I do?

Anyway, by all means create a "microlog not empty" flag for each command
and Post a warning at the end if it's set.........

ADB

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

No branches or pull requests

2 participants